This repository was archived by the owner on Dec 15, 2022. It is now read-only.
Upgrade bindings to define transient types #111
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug
When trying to write jest tests for a library built on top of oniguruma, tests often failed with the error:
Which I tracked to this line:
node-oniguruma/src/oniguruma.js
Line 108 in aa7f727
This line adds some additional functionality to the native library imported, before exporting the result. It does so by directly modifying the prototype chain of the native types. If this module is evaluated once, that behavior is safe. But in case where the module cache is updated/modified as the tests are running, reevaluating that module will fail the process.
Description of the Change
To fix this, and enable using this library safely when writing tests, I propose updating the binding to export child types with the augmented functionality. Not only it is more readable, it guards against such failures, by exporting a new type every time.
Possible Drawbacks
First time contibutor, so I'd welcome guidance here.
Verification Process
Release Notes
N/A - a refactor with no user-facing changes.