Support for Node 10.x #108
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Aron Carroll <aron.carroll@snyk.io>
Signed-off-by: Aron Carroll <aron.carroll@snyk.io>
1 task
|
Hey @srenatus, would it be possible to publish a new version (or an alpha/beta release) of the package containing the latest changes for node 10? |
|
Will do 👍 |
|
@aron done! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I've run into an issue where version 1.2.0 of opa-wasm has lost support for Node 10. This is due to the change introduced in #45 to check for the
wasm.instance.exports.opa_wasm_abi_version. On Node 12+ this value is returned as an object with a numericvalueproperty. On Node 10 it's a numeric literal. This results in anunsupported ABI version undefinederror when trying to load any module. Otherwise everything works as expected. I've not looked into detail for the reasons for the change, nothing is mentioned in the NodeJS wasm documentation.I've opened this PR to fix the issue so that the tool supports Node 10.x, which required a type check on the
opa_wasm_abi_versionvalue in opa.js. I understand that Node 10 is an extremely old version that is already past end-of-life so I'm not asking to officially support it but wondered if you'd consider taking on the patch regardless? At Snyk where we use opa-wasm in our CLI we're still currently committed to supporting Node 10 at least for the next quarter while we phase it out.For the purposes of the PR I've enabled the test matrix to include Node 10, I propose that if you approve the change I'll revert the change to the GitHub action before merging. Cheers.