Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conditional export for cjs and es6 entry points #741

Merged
merged 1 commit into from Feb 8, 2023

Conversation

neeerp
Copy link
Contributor

@neeerp neeerp commented Feb 7, 2023

Issue #, if available: #740

Description of changes:

Added conditional exports to package.json for the CommonJS and ESModule builds respectively. See node docs on conditional exports.

Tested the commonjs export by using my node REPL and noting that require.resolve('ion-js') returns node_modules/ion-js/dist/commonjs/es6/Ion.js.

Tested the es6 export by creating a small project containing the code snippet example from the Getting Started section of the README under src/index.js, and using rollup to build the project. In the tree shaken node_modules generated by rollup, I can see only ion-js/dist/es6/es6 is preserved.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jobarr-amzn
Copy link
Contributor

This seems good to me. The ion-test-driver workflow fails, but looking back in the history of ion-js pull requests I see that this workflow has been consistently failing for a while.

Futhermore, the persistent failure seems to be related to python setup trouble not ion-js issues.

@jobarr-amzn
Copy link
Contributor

Created #742 based on failing PR workflow.

@jobarr-amzn
Copy link
Contributor

jobarr-amzn commented Feb 7, 2023

@jobarr-amzn jobarr-amzn merged commit 03384c3 into amazon-ion:master Feb 8, 2023
@everett1992
Copy link
Contributor

This seems to not work in node, because ion-js's esm doesn't follow node's requirements.
#764

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants