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

replace import specifier with module #767

Merged
merged 3 commits into from Sep 6, 2023
Merged

Conversation

everett1992
Copy link
Contributor

@everett1992 everett1992 commented Aug 26, 2023

fixes #764

Prior to this commit ion-js declared a 'import' conditional export, which meant that node would load ./dist/es6/es6/Ion.js when loaded via import.

However, ion-js's es6 output is not compatible with node because it doesn't use the correct file extension or package.json type to indicate it is a esm file, and it's imports do not use file extensions. Node uses the import condition whenever a module is loaded with import.

Long term it would be good to convert to esm or use import but that would require writing and emitting valid ESM, I had some issues with that that I described in the original issue.

As a quick fix I replaced the import condition with a module condition, which bundlers like esbuild and webpack will prefer.

esbuild uses es6

echo "import 'ion-js'" | npx esbuild --bundle --analyze --outfile=/dev/null
  ../../../../../dev/null                      175.2kb  100.0%
   ├ dist/es6/es6/IonParserTextRaw.js           41.5kb   23.7%

node can import and require ion-js

node -e "require('ion-js')"
node -e "import('ion-js')"

https://nodejs.org/docs/latest-v16.x/api/packages.html#subpath-exports https://nodejs.org/docs/latest-v16.x/api/packages.html#conditional-exports https://nodejs.org/docs/latest-v16.x/api/esm.html#mandatory-file-extensions https://esbuild.github.io/api/#how-conditions-work


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

fixes amazon-ion#764

Prior to this commit ion-js declared a 'import' conditional export,
which meant that node would load `./dist/es6/es6/Ion.js` when loaded via
import.

However, ion-js's es6 output is not compatible with node because it
doesn't use the correct file extension or package.json `type` to
indicate it is a esm file, and it's imports do not use file extensions.
Node uses the `import` condition whenever a module is loaded with
`import`.

Long term it would be good to convert to esm or use `import` but that
would require writing and emitting valid ESM, I had some issues with
that that I described in the original issue.

As a quick fix I replaced the `import` condition with a `module`
condition, which bundlers like esbuild and webpack will prefer.

esbuild uses es6
```
echo "import 'ion-js'" | npx esbuild --bundle --analyze --outfile=/dev/null
  ../../../../../dev/null                      175.2kb  100.0%
   ├ dist/es6/es6/IonParserTextRaw.js           41.5kb   23.7%
```

node can import and require ion-js
```
node -e "require('ion-js')"
node -e "import('ion-js')"
```

https://nodejs.org/docs/latest-v16.x/api/packages.html#subpath-exports
https://nodejs.org/docs/latest-v16.x/api/packages.html#conditional-exports
https://nodejs.org/docs/latest-v16.x/api/esm.html#mandatory-file-extensions
https://esbuild.github.io/api/#how-conditions-work
Copy link
Contributor

@desaikd desaikd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@desaikd desaikd merged commit 7fb8986 into amazon-ion:master Sep 6, 2023
7 checks passed
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.

ion-js@5.1.0 is incompatable with node import ecmascript modules
2 participants