material-foundation / material-color-utilities

Color libraries for Material You
Apache License 2.0
1.57k stars 134 forks source link

Module reports as CJS despite being ESM #83

Closed WillsterJohnson closed 1 year ago

WillsterJohnson commented 1 year ago

When importing from the TS lib into an ESM file Node gives the following error;

import { argbFromLab, labFromArgb, themeFromSourceColor } from "@material/material-color-utilities";
import { argbFromLab, labFromArgb, themeFromSourceColor } from "@material/material-color-utilities";
         ^^^^^^^^^^^
SyntaxError: Named export 'argbFromLab' not found. The requested module '@material/material-color-utilities' 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 '@material/material-color-utilities';
const { argbFromLab, labFromArgb, themeFromSourceColor } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

Node.js v18.14.0
 ELIFECYCLE  Command failed with exit code 1.

After following it's advice;

import pkg from "@material/material-color-utilities";
const { argbFromLab, labFromArgb, themeFromSourceColor } = pkg;
(node:1016168) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/media/will/NVMe250/Projects/mess-with-m3/node_modules/.pnpm/@material+material-color-utilities@0.2.2/node_modules/@material/material-color-utilities/dist/index.js:17
export * from './blend/blend.js';
^^^^^^

SyntaxError: Unexpected token 'export'
    at internalCompileFunction (node:internal/vm:73:18)
    at wrapSafe (node:internal/modules/cjs/loader:1149:20)
    at Module._compile (node:internal/modules/cjs/loader:1190:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1280:10)
    at Module.load (node:internal/modules/cjs/loader:1089:32)
    at Module._load (node:internal/modules/cjs/loader:930:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:169:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

Node.js v18.14.0
 ELIFECYCLE  Command failed with exit code 1.

Current workaround; Go into the node modules folder, find the package.json for this module, add the line "type": "module",

Proposed permanent solution; Set "type": "module" in the package.json file.

Alternatives considered; Target typescript at CJS. I strongly dislike this idea personally as ESM was introduced some time ago with the intention of it replacing CJS after the ECMA team decided ESM was an overall objectively better syntax than CJS. As such, I would argue that going back to CJS is roughly equivalent to swapping out let and const for var, or classes for prototype monsters. Additionally, CJS libraries often don't play nice with frontend frameworks, which is likely going to be a large portion of this library's usage. It might work, though it might also alienate a notable chunk of users. TL:DR; CJS is hassle, ESM is less hassle.

Note: despite #82 I can confirm this is unrelated as I manually fixed 82 in my node modules folder before encountering this module type problem

rodydavis commented 1 year ago

We did have "type":"module" in he past and it broke a lot of installs.

I think having exports with both CJS and ESM modules should work.

https://github.com/material-components/material-web also had the same issue, but added back the module to the key.

Will do some tests. We use modules and ESM, but will check the publish process to see what is being published

WillsterJohnson commented 1 year ago

with the newer package.json exports field it should be possible to do both, the way typescript handles builds makes it a bit of a pain. I think I was tinkering with another lib a while back and found that a secondary tsconfig which extends the main one and targets the other module type is a "good enough" solution, though I've seen other libs do things differently. Currently it can't be imported by either module type so hopefully it won't cause too much trouble to get it working.

rodydavis commented 1 year ago

This is the internal change that will fix it: https://github.com/material-foundation/material-color-utilities/pull/84

Did a test locally with the new output and works much better with typescript and esm right out of the box.

rodydavis commented 1 year ago

Honestly dropping CJS for ESM is fine, and many other libraries are doing the same

WillsterJohnson commented 1 year ago

looks great, fixes #82 as well! As far as I can tell those changes will fix the remaining issues with the TS lib as far as functionality goes. Thank you for getting to this so quickly

WillsterJohnson commented 1 year ago

0.2.3 fixes this