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 ESM support #58

Merged
merged 2 commits into from
Sep 12, 2021
Merged

Fix ESM support #58

merged 2 commits into from
Sep 12, 2021

Conversation

alecmev
Copy link
Contributor

@alecmev alecmev commented Sep 10, 2021

Proposed changes

If you try running code that name-imports svg-pathdata using something like node --loader ts-node/esm foo.ts, you would get the following error:

import { SVGPathData } from 'svg-pathdata';
         ^^^^^^^^^^^
SyntaxError: Named export 'SVGPathData' not found. The requested module 'svg-pathdata' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'svg-pathdata';
const { SVGPathData } = pkg;

This PR fixes this. I'm no module/bundler specialist though, so I don't know what to test, so have no idea if this is going to break anything for anybody. "type": "module" certainly has the explosive potential.

Node's ESM docs.

Code quality

  • I made some tests for my changes
  • I added my name in the
    contributors
    field of the package.json file

License

To get your contribution merged, you must check the following.

  • I read the project license in the LICENSE file
  • I agree with publishing under this project license

@pioug
Copy link
Collaborator

pioug commented Sep 10, 2021

Did a quick test and it seems like "type": "module" can break existing projects.

$ node test.js 
/Users/piou/Dev/svgest/test.js:1
const { SVGPathData } = require('svg-pathdata');
                        ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/piou/Dev/svg-pathdata/lib/SVGPathData.js from /Users/piou/Dev/svgest/test.js not supported.
SVGPathData.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead rename SVGPathData.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /Users/piou/Dev/svg-pathdata/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

I think using .cjs would be good enough. By changing lib/SVGPathData.js to lib/SVGPathData.cjs, we will do something similar to the example in https://nodejs.org/api/packages.html#packages_approach_2_isolate_state (though .mjs doesn't seem necessary).

@alecmev
Copy link
Contributor Author

alecmev commented Sep 11, 2021

Thank you for looking into this! Would you like me to make that change?

@pioug
Copy link
Collaborator

pioug commented Sep 12, 2021

I think so, especially if you want the attribution. I can't seem to modify your PR. You can take this commit b65b57d. Or I can push directly to the main branch and close the PR. Up to you!

@alecmev
Copy link
Contributor Author

alecmev commented Sep 12, 2021

I can push directly to the main branch and close the PR

This is okay for me, no need to attribute 😉 Thank you!

Edit: Adding commits to this PR should work though, I have "Allow edits by maintainers" checked.

@pioug pioug merged commit c3251d2 into nfroidure:master Sep 12, 2021
@MaikuMori
Copy link

MaikuMori commented Sep 13, 2021

This didn't fix it. I guess the index.d.ts is messing things up. Why is that file even needed in a TypeScript project?

Edit:
Here's a decent writeup. Type information could be rolled up or not, but currently index.d.ts is saying import types from /lib, but there is no type information there.

@alecmev alecmev deleted the patch-1 branch February 25, 2022 21:50
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.

3 participants