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

ESM module export differs from main export #240

Open
lygstate opened this issue Oct 10, 2020 · 26 comments · May be fixed by #243
Open

ESM module export differs from main export #240

lygstate opened this issue Oct 10, 2020 · 26 comments · May be fixed by #243
Assignees
Labels
Milestone

Comments

@lygstate
Copy link

lygstate commented Oct 10, 2020

Webpack v4+ prefers the module field from package.json over the main field even when the webpack target is node.

Since the main module exports {parse, stringify} and the module module exports {default: {parse, stringify}}, this creates an issue when bundling with webpack.

@TrySound
Copy link

Using named exports would solve all problems.

@lygstate
Copy link
Author

There is objection, https://www.bignerdranch.com/blog/default-exports-or-named-exports-why-not-both/
I am bitten by these things.

maybe webpack better to provide a optionMap to config each node module are default export or named export.

@lygstate
Copy link
Author

Using named exports would solve all problems.

Convice me doesn't solve the problem, need convnce everyone and that's not possible.

@TrySound
Copy link

In this particular case technically these are named exports. There is no "primary" thing here and combining into single object does not solve any problem. If you will need single object use special syntax import * as JSON5 from 'json5'.
https://github.com/json5/json5/blob/master/lib/index.js#L4-L7

@lygstate
Copy link
Author

In this particular case technically these are named exports. There is no "primary" thing here and combining into single object does not solve any problem. If you will need single object use special syntax import * as JSON5 from 'json5'.
https://github.com/json5/json5/blob/master/lib/index.js#L4-L7

These are right, but the generated mjs code are default export

@lygstate
Copy link
Author

lygstate commented Oct 10, 2020

Look at the dist file index.mjs, it's ends with default export, that's truely annoy.

const JSON5 = {
    parse,
    stringify,
};

var lib = JSON5;

export default lib;

I using

Adjust webpack mainFields: to ['main', 'module'] for supporting newest
json5. to fix. but this is too much

TrySound added a commit to TrySound/json5 that referenced this issue Oct 10, 2020
In this diff I added new entry point to compile es modules with named
exports to solve this problem (json5#240).

It's a breaking change so we can drop legacy node support (newer
versions of rollup and many other tools requires this) and add native
node es modules support.
@TrySound
Copy link

Check this #241

@lygstate lygstate changed the title json5 distrubte e "module": "dist/index.mjs", hurt webpack json5 distrubte "module": "dist/index.mjs", hurt webpack Oct 10, 2020
@jordanbtucker
Copy link
Member

@lygstate Can you please explain what your issue is rather than just adding a link to an issue in another repo.

@TrySound
Copy link

The issue is using the library in commonjs code which is bundled by webpack

// webpack resolves json5 with module field out of the box which lead to this usage
const json5 = require('json5').default

Though this does not work in node which does not have "default" field.

@jordanbtucker jordanbtucker changed the title json5 distrubte "module": "dist/index.mjs", hurt webpack ESM module export differs from main export Oct 12, 2020
@jordanbtucker
Copy link
Member

Would it work to add an exports field to package.json as described in Dual CommonJS/ES module packages and Target environment independent packages?

@TrySound
Copy link

It won't work with webpack 4 which supports only "module" field. Do you have any objections to use named exports instead?
https://github.com/json5/json5/pull/241/files

I can migrate whole source to esm in the next PR if you want.

@jordanbtucker
Copy link
Member

I'm trying to avoid breaking changes.

@jordanbtucker jordanbtucker self-assigned this Oct 12, 2020
@jordanbtucker jordanbtucker linked a pull request Oct 12, 2020 that will close this issue
@jordanbtucker
Copy link
Member

jordanbtucker commented Oct 12, 2020

What about just assigning values to exports instead of reassigning module.exports like I did in #243? That should produce named exports along with the default export. I don't think that constitutes a breaking change.

It produces the following code for index.mjs.

var parse_1 = parse;
var stringify_1 = stringify;

var lib = {
	parse: parse_1,
	stringify: stringify_1
};

export default lib;
export { parse_1 as parse, stringify_1 as stringify };

@TrySound
Copy link

TrySound commented Oct 12, 2020

Well, yeah, we can keep both until next major.
AFAIK generating exports may not work with newer rollup version. Safer to have separate entry point with explicit (not generated) exports.
I reverted breaking changes here
#241

@ilanc
Copy link

ilanc commented Feb 23, 2022

Hi there,
What's the correct way to import json5 from esm code? See code below and error:

I get:

  • 'json5' is a CommonJS module if I use import { parse } from "json5";
  • it works with default import import json5 from "json5"; - but is my code using json5 cjs or esm code?
import json5 from "json5";
// import { parse } from "json5";

let json = `{
  // comment
   a: 1,
   b: 1,
   c: 1
}`;
console.log("json5.parse:", JSON.stringify(json5.parse(json)));
// console.log("parse:", JSON.stringify(parse(json))); // 'json5' is a CommonJS module??

@ilanc
Copy link

ilanc commented Feb 23, 2022

hmm .. appears to be using cjs code
image

@ilanc
Copy link

ilanc commented Feb 23, 2022

Hi @jordanbtucker, please checkout this sample repo: https://github.com/ilanc/esm-json5 and let me know what you think.

There's a clear problem with trying to use ESM json5 in node: i.e. node will use the CJS version of json5 even when included inside .mjs code. I've tested this with the following node versions:

  • v14.16.0
  • v16.4.2

The fix is to add and "exports" section to package.json. NB I know CJS/ESM transition is a pain and it's not easy to avoid breaking changes. will this "exports" break other js environments? NB the current "browser" looks wrong anyway = points to the bundled CJS code.

"exports": {
    ".": {
      "require": "./dist/index.js",
      "import": "./dist/index.mjs"
    }
  },

@jordanbtucker jordanbtucker added this to the v3.0.0 milestone Feb 23, 2022
@jordanbtucker
Copy link
Member

jordanbtucker commented Feb 23, 2022

@ilanc

What's the correct way to import json5 from esm code?

The current correct way is to import JSON5 from 'json5/dist/index.mjs, however this is not documented.

In April, when Node.js v12 is no longer supported, I plan on releasing json5@3, which will include your proposed fix and a few other breaking changes (moving the CLI to its own package, removing or deprecating require('json5/lib/register'), performance improvements. Non-breaking changes may be backported to json5@2.

@ilanc
Copy link

ilanc commented Feb 24, 2022

Makes sense, thanks for the reply

@ilanc
Copy link

ilanc commented Feb 24, 2022

@jordanbtucker do you have a workaround for typescript? see error below

// esm.ts
import json5 from "json5/dist/index.mjs";
let json = `{
  // comment
   a: 1,
   b: 1,
   c: 1
}`;
console.log("json5.parse:", JSON.stringify(json5.parse(json)));

gives this error (I have no tsconfig.json in the directory atm):

$ tsc esm.ts 
esm.ts:2:19 - error TS2307: Cannot find module 'json5/dist/index.mjs' or its corresponding type declarations.

2 import json5 from "json5/dist/index.mjs";
                    ~~~~~~~~~~~~~~~~~~~~~~


Found 1 error.

@ilanc
Copy link

ilanc commented Feb 24, 2022

I was using tsc Version 4.3.5. It appears to work without needing any special tsconfig with Version 4.5.5

@jordanbtucker
Copy link
Member

jordanbtucker commented Feb 24, 2022

@ilanc I wonder if copying index.d.ts into node_modules/json5/dist will resolve that error. If so, that would be an easy short term fix that I could publish. It might need to be renamed to index.mjs.d.ts.

@ilanc
Copy link

ilanc commented Feb 25, 2022

@jordanbtucker possibly, and it may need types - but I wouldn't bother it'll change again with the next typescript / node / bundler / linter / ... sigh

@ilanc
Copy link

ilanc commented Feb 25, 2022

@jordanbtucker btw - not sure whether you've read these - but I've found these to be very useful articles on building dual cjs/esm packages:

@jordanbtucker
Copy link
Member

jordanbtucker commented Feb 25, 2022

I wonder if copying index.d.ts into node_modules/json5/dist will resolve that error. If so, that would be an easy short term fix that I could publish. It might need to be renamed to index.mjs.d.ts.

I checked. It's not that simple. index.d.ts uses export {...} while index.mjs uses export default, which is the original point of this issue. Plus, index.d.ts imports ./parse.d.ts and ./stringify.d.ts so those paths don't resolve correctly.

The following declaration placed at dist/index.mjs.d.ts works though (no need for @types):

import parse = require('../lib/parse')
import stringify = require('../lib/stringify')

interface JSON5 {
  parse: typeof parse
  stringify: typeof stringify
}

declare const JSON5: JSON5
export default JSON5

Is there a specific reason you need the ESM version in TypeScript?


NB the current "browser" looks wrong anyway = points to the bundled CJS code.

Both dist/index.js and dist/index.mjs are bundled from the CJS code, but dist/index.js is not CJS code, it's UMD code. If you want to import json5 inside <script type="module"> in the browser, then you would use dist/index.mjs.

<!-- Import json5 as a UMD global -->
<script src="https://unpkg.com/json5@2/dist/index.js"></script>
<script>
// JSON5 is a global
console.log(JSON5.parse(`{hello: 'world'}`))
</script>

or

<!-- Import json5 as a module -->
<script type="module">
import JSON5 from 'https://unpkg.com/json5@2/dist/index.mjs'
console.log(JSON5.parse(`{hello: 'world'}`))
</script>

@ilanc
Copy link

ilanc commented Feb 25, 2022

I don't want to keep wasting your time with this stuff. Thanks for all the responses.

Is there a specific reason you need the ESM version in TypeScript?

Nope. I'm going through a painful exercise of porting my codebase from cjs to esm. One of the changes is to resolve how the 3rd party packages that I'm using have implemented esm. Of the 50+ packages I'm using I picked json5 to single step into and realised that the code tranformation suggested by node (i.e. import json5 from "json5"; const { parse } = json5;) wasn't doing what I thought it would do - i.e. using the esm code. That's the reason why I wound up here. Whether it would have made a difference I don't know - it was just unexpected - so I dug a bit deeper.

I'm totally happy with the import json5 from "json5/dist/index.mjs" + ts4.5+ solution.

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

Successfully merging a pull request may close this issue.

4 participants