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

fix: add .js extension to imports and fix package.json #148

Conversation

achingbrain
Copy link

This fixes two problems with the ESM build of this module.

  1. The package.json that contains { "type": "module" } wasn't being included in the npm tarball
  2. When running in an ESM environment, import foo from './bar' does not work, you have to specify the extension

The fix for the first is simple, add the cjs/esm package.json files to the files array in the project package.json.

The second fix is harder. If you just add the .js extension to the source files, typescript is happy but ts-node is not, and this project uses ts-node to run the tests without a compile step.

Typescript does not support importing *.ts and will not support adding *.js to the transpiled output - microsoft/TypeScript#16577

ts-node thought this was a bug in Typescript but it turns out not. Their suggestion to use ts-node/esm breaks sourcemap support because source-map-support/register is not esm - TypeStrong/ts-node#783

There is a PR against ts-node to add support for resolving ./foo.js if ./foo.ts or ./foo fails but it seems to have stalled - TypeStrong/ts-node#1361

Given all of the above, the most expedient way forward seemed to just be to add a shell script that rewrites the various import statements in the esm output to add the .js extension, then if the ts-node PR ever gets merged the script can be backed out.

Fixes #147

This fixes two problems with the ESM build of this module.

1. The `package.json` that contains `{ "type": "module" }` wasn't being included in the npm tarball
2. When running in an ESM environment, `import foo from './bar'` does not work, you have to specify the extension

The fix for the first is simple, add the cjs/esm `package.json` files to the `files`
array in the project `package.json`.

The second fix is harder.  If you just add the `.js` extension to the source files,
typescript is happy but ts-node is not, and this project uses ts-node to run the
tests without a compile step.

Typescript does not support importing `*.ts` and will not support adding `*.js` to
the transpiled output - microsoft/TypeScript#16577

ts-node thought this was a bug in Typescript but it turns out not.  Their suggestion
to use `ts-node/esm` breaks sourcemap support because `source-map-support/register`
is not esm - TypeStrong/ts-node#783

There is a PR against ts-node to add support for resolving `./foo.js` if `./foo.ts`
or `./foo` fails but it seems to have stalled - TypeStrong/ts-node#1361

Given all of the above, the most expedient way forward seemed to just be to add
a shell script that rewrites the various `import` statements in the esm output
to add the `.js` extension, then if the ts-node PR ever gets merged the script
can be backed out.

Fixes beaugunderson#147
@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #148 (1a8d3f0) into master (b1df15f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #148   +/-   ##
=======================================
  Coverage   98.08%   98.08%           
=======================================
  Files           9        9           
  Lines         575      575           
  Branches       84       84           
=======================================
  Hits          564      564           
  Misses          3        3           
  Partials        8        8           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1df15f...1a8d3f0. Read the comment docs.

@achingbrain
Copy link
Author

@beaugunderson are there any changes you'd like made to this PR or can it be merged?

#!/bin/sh

# Search for "from './baz'" and transform to "from './baz.js'"
find dist/esm -type f -name *.js -exec sed -i -e -E "s/from '(\.\/.*)'/from '\1.js'/g" {} \;

Choose a reason for hiding this comment

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

Tested on debian the -e parameter doesn't work.

Copy link

Choose a reason for hiding this comment

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

also this won't catch any pure imports like import 'utils/myfile.js'

Copy link

Choose a reason for hiding this comment

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

Note for anyone else viewing this that I tried getting a rule into eslint that would resolve this for similar use-cases: import-js/eslint-plugin-import#2701

@beaugunderson
Copy link
Owner

removed all the ESM-specific stuff in this project, tested that the new version works in CJS/ESM/TS without issues... publishing a new release momentarily

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.

Address4 Not Found
5 participants