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

[Breaking] add native ESM entry point #358

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@d-fischer
Copy link

d-fischer commented Mar 12, 2020

This fixes the following error when importing using the native ESM loader in node:

import { stringify } from 'qs';
         ^^^^^^^^^
SyntaxError: The requested module 'qs' does not provide an export named 'stringify'

It should also enable tree shaking.

@@ -11,6 +11,12 @@
"url": "https://github.com/sponsors/ljharb"
},
"main": "lib/index.js",
"exports": {
".": {

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 12, 2020

Owner

this needs the array fallback to the string form, or it won't work on node 13.0 or 13.1.

This comment has been minimized.

Copy link
@d-fischer

d-fischer Mar 12, 2020

Author

Can you show me a documentation of this so I can implement it in the PR? I actually haven't heard about this.

@@ -11,6 +11,12 @@
"url": "https://github.com/sponsors/ljharb"
},
"main": "lib/index.js",
"exports": {

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 12, 2020

Owner

adding "exports" is a breaking change. this should also include the ability to access package.json.

This comment has been minimized.

Copy link
@d-fischer

d-fischer Mar 12, 2020

Author

People actually access other people's package.json and assume it's a part of the public API of packages? Never heard of that

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 13, 2020

Owner

Everything reachable is public, yes - and the lack of package.json access broke preact users recently.

import parse from './parse.js';
import formats from './formats.js';

export default {

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 12, 2020

Owner

there's no value or point in the default export if you have the names :-)

This comment has been minimized.

Copy link
@d-fischer

d-fischer Mar 12, 2020

Author

I was thinking that people could do this, like they currently can with the help of bundlers:

import qs from 'qs';

qs.parse('a=1');

And I think this PR might actually not be a breaking change because of this.

@ljharb

This comment has been minimized.

Copy link
Owner

ljharb commented Mar 12, 2020

Note that you can already get the benefits of "tree shaking" by directly importing qs/lib/parse or qs/lib/stringify; "tree shaking" only provides benefits when importing more than you need in the first place.

@ljharb

This comment has been minimized.

Copy link
Owner

ljharb commented Mar 12, 2020

I've gone ahead and updated the PR to be mergeable, but I'm going to hold off until something else forces a v7.

@ljharb ljharb force-pushed the d-fischer:esm branch from 670656f to 8ee4245 Mar 12, 2020
@ljharb ljharb changed the title [new] add native ESM entry point [Breaking] add native ESM entry point Mar 12, 2020
@d-fischer

This comment has been minimized.

Copy link
Author

d-fischer commented Mar 12, 2020

Note that you can already get the benefits of "tree shaking" by directly importing qs/lib/parse or qs/lib/stringify; "tree shaking" only provides benefits when importing more than you need in the first place.

I think the usual best practice is that you only import from the entry point, which generally includes everything.

@ljharb

This comment has been minimized.

Copy link
Owner

ljharb commented Mar 13, 2020

That’s the usual practice, yes, but it’s not “best” whatsoever :-)

Co-authored-by: Daniel Fischer <daniel@d-fischer.dev>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@d-fischer d-fischer force-pushed the d-fischer:esm branch from 8ee4245 to 8240c36 Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.