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

Multiple issues with ES module #50

Closed
tkrotoff opened this issue Jul 5, 2022 · 5 comments · Fixed by #57
Closed

Multiple issues with ES module #50

tkrotoff opened this issue Jul 5, 2022 · 5 comments · Fixed by #57

Comments

@tkrotoff
Copy link

tkrotoff commented Jul 5, 2022

module is obsolete

package.json specifies:

"module": "dist/clsx.m.js",
"main": "dist/clsx.js",

The proper way is:

"type": "module",
"main": "dist/clsx.cjs",   // or .js if "type" is unspecified
"exports": {
  "import": "./dist/clsx.js",   // no need for .mjs if "type" is "module" (recommended)
  "require": "./dist/clsx.cjs"   // or .js if "type" is unspecified
},

Related issue: lukeed/bundt#7

.m.js extension should be .mjs

https://nodejs.org/docs/latest-v16.x/api/packages.html#determining-module-system

(.mjs is only necessary if "type": "module" is not specified)

Syntax import { clsx } from 'clsx' does not work in an app transpiled to CJS

clsx is undefined

TypeError: (0 , clsx__WEBPACK_IMPORTED_MODULE_0__.clsx) is not a function

Last line from dist/clsx.js is exports.clsx = clsx and exports comes from nowhere, I don't think this can work. Caused by https://github.com/lukeed/bundt I guess.

Solution I've found for import { clsx } from 'clsx' to work in an app transpiled to CJS

With webpack, create an alias clsx: 'clsx/dist/clsx.m.js', example with Next.js webpack config from next.config.js:

  // ...

  webpack: (config, options) => {
    config.resolve.alias = {
      ...config.resolve.alias,
      clsx: 'clsx/dist/clsx.m.js'
    };

   // ...

This will force the import of dist/clsx.m.js instead of dist/clsx.js. (You will need to transpile node_modules/clsx with next-transpile-modules in the case of Next.js).





Yep, ES Module is a real challenge and a big mess :-/

@lukeed
Copy link
Owner

lukeed commented Jul 5, 2022

This is not CommonJS:

import { clsx } from 'clsx';

the import is exclusively ESM syntax. You’re looking for this:

const { clsx } = require('clsx');
// or
const clsx = require('clsx');

those are your only options with CommonJS

@lukeed lukeed closed this as completed Jul 5, 2022
@tkrotoff
Copy link
Author

tkrotoff commented Jul 5, 2022

This is not CommonJS

This code will be transpiled to CommonJS 👌

the import is exclusively ESM syntax

exclusively ? Transpilers allow to use the import syntax and generate CJS (require) code.

This is how 95% of developers code and have done so for almost a decade.

ESM is activated with "type": "module" or .mjs, everything else is CommonJS (with or without lipstick).


I think there is a misunderstanding here.

lukeed added a commit that referenced this issue Jul 5, 2022
- mixed exports (named + default) broke old bundt... because it's discouraged
- closes #50
@lukeed
Copy link
Owner

lukeed commented Jul 6, 2022

I saw what you meant re: the broken CommonJS file ... fixed it & the UMD file

However:

exclusively ?

Yes. There's a misunderstanding here & with your initial commit. The file definitions are fine & your thinking regarding requiring type: module, etc is not entirely correct either. It definitely is messy, but the package itself is defined correctly. A future 2.0.0 version will make use of the "exports" mapping, which allows native ESM imports, but that's not (currently) related to any of this & definitely not related to the problem you were experiencing.

This is the footer of the [email protected]/dist/clsx.js file:

// ...
module.exports = clsx;
exports.clsx = clsx;

Thru CommonJS semantics, this means that the only declaration that takes effect is module.exports = clsx, which means that there's only a ~"default" (not accurate) function export... no named clsx export.

The exports.clsx = clsx is normally fine, but only when there are only named exports involved.

This fixes it for the next release:

module.exports = clsx;
module.exports.clsx = clsx;

Separately, your bundler configuration is pretty whacky. It doesn't matter if you're building for CommonJS – if you're still going to author/write your source files in ESM (import/export), then you should be configuring your bundler to look at ESM-targeted fields.

This means that you should have automatically picked up the clsx.m.js file automatically instead of having to manually resolve it.

You should only ever load the clsx.js file (CommonJS) if you do require('clsx'). Your bundler is smart enough to convert:

import { clsx } from 'clsx';
// into
const { clsx } = require('clsx');

when building for & targeting CJS output.

Granted, yes, you still would have had an issue with const { clsx } = require('clsx') here, but only because the file's exports were overwriting each other.


Hope that helps~! Will publish patch shortly.

@tkrotoff
Copy link
Author

tkrotoff commented Jul 6, 2022

Version 1.2.1 fixes the issue I had, thanks a lot!

I did some experimentations with 1.2.1.

  • Next.js uses node_modules/clsx/dist/clsx.js probably because of SSR/getServerSideProps
  • create-react-app uses node_modules/clsx/dist/clsx.m.js

(You can reproduce this by generating the default projects, import clsx and add console.logs to the different files inside node_modules/clsx/dist/)

It's like create-react-app recognizes "module": "dist/clsx.m.js" while Next.js does not.

@tkrotoff
Copy link
Author

tkrotoff commented Jul 8, 2022

which allows native ESM imports, but that's not (currently) related to any of this & definitely not related to the problem you were experiencing.

Got it, the pattern you are currently following is https://github.com/rollup/rollup/wiki/pkg.module and this is still relevant.

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 a pull request may close this issue.

2 participants