material-foundation / material-color-utilities

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

[Bug][Inconsequential] Deprecation Warning - no main or exports defined #85

Closed WillsterJohnson closed 1 year ago

WillsterJohnson commented 1 year ago

Inconsequential issue, might break for node v20+

There is now a deprecation warning issued by node when using this library;

(node:1085060) [DEP0151] DeprecationWarning: No "main" or "exports" field defined in the package.json for /media/will/NVMe250/Projects/mess-with-m3/node_modules/@material/material-color-utilities/ resolving the main entry point "index.js", imported from /media/will/NVMe250/Projects/mess-with-m3/dist/index.js.
Default "index" lookups for the main are deprecated for ES modules.
(Use `node --trace-deprecation ...` to show where the warning was created)

With --trace-deprecation;

(node:1085529) [DEP0151] DeprecationWarning: No "main" or "exports" field defined in the package.json for /media/will/NVMe250/Projects/mess-with-m3/node_modules/@material/material-color-utilities/ resolving the main entry point "index.js", imported from /media/will/NVMe250/Projects/mess-with-m3/dist/index.js.
Default "index" lookups for the main are deprecated for ES modules.
    at emitLegacyIndexDeprecation (node:internal/modules/esm/resolve:144:13)
    at legacyMainResolve (node:internal/modules/esm/resolve:231:5)
    at packageResolve (node:internal/modules/esm/resolve:876:14)
    at moduleResolve (node:internal/modules/esm/resolve:938:20)
    at defaultResolve (node:internal/modules/esm/resolve:1153:11)
    at nextResolve (node:internal/modules/esm/loader:163:28)
    at ESMLoader.resolve (node:internal/modules/esm/loader:838:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:424:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:77:40)
    at link (node:internal/modules/esm/module_job:76:36)

Disappears when adding either of the following entries to package.json;

  "main": "index.js",
  // or
  "exports": {
    ".": "./index.js"
  },
kwaa commented 1 year ago

Unexpected. I'd recommend:

"main": "index.js",
"module": "index.js",
"types": "index.d.ts",
"exports": {
  ".": {
    "types": "./index.d.ts",
    "import": "./index.js"
  }
}
rodydavis commented 1 year ago

Oh interesting, I was following a similar package we publish on material web: https://github.com/material-components/material-web/blob/master/package.json

Shouldn't be an issue to add!

WillsterJohnson commented 1 year ago

I'd hazzard a guess that the same warning will show on that package too. Looks like this deprecation was introduced in Node14, with the terminal warning being added in Node16 https://nodejs.org/api/deprecations.html#DEP0151 Given it's age it's probably due to be an actual error in Node20, though Node19 still has it as a depreaction warning so it might be Node21/22 when it's actually dropped.

Still, better to resolve it now than have users' code break when it's dropped.


Somewhat off topic; is there a planned date/estimate for this repo being open to contributions? I feel like it would be beneficial to the team and to the users if the little things like this went directly to a PR that could be merged and released quickly rather than going through the process of opening and issue, changing it internally, running the mirror automations, etc. It would also save the team time that could be focused on more important things than config files. There are also some issues around which I'd like to contribute a resolution to but am holding off on due to the whole read-only mirror thing.

WillsterJohnson commented 1 year ago

@kwaa when I was adding to the package.json in node modules I found that TS v4.9 picked up the types without needing to specify more than either the main or exports field in the original comment. I may have missed something but it seems to work okay without them. Are they needed for something else that I haven't looked at?

kwaa commented 1 year ago

@kwaa when I was adding to the package.json in node modules I found that TS v4.9 picked up the types without needing to specify more than either the main or exports field in the original comment. I may have missed something but it seems to work okay without them. Are they needed for something else that I haven't looked at?

Writing more is always less likely to cause problems, for example in bundlers or older versions of TypeScript.

The TypeScript documentation still recommends that types be written to the export map and placed first - for more, take a look at the publint rules.

WillsterJohnson commented 1 year ago

Ah gotcha, backwards compatibility needing extras as usual. If the docs say to do it then I guess it only makes sense to do it.

rodydavis commented 1 year ago

Working on adding it to an internal change to roll out!

rodydavis commented 1 year ago

Published and fixed!

flyon commented 1 year ago

Is this related guys or should I open a new issue? I'm trying to import this in my typescript project

import { argbFromHex, themeFromSourceColor, applyTheme } from "@material/material-color-utilities";

and I get this error:

Module not found: Error: Package path . is not exported from package /web/workspace/node_modules/@material/material-color-utilities (see exports field in /web/workspace/node_modules/@material/material-color-utilities/package.json)
flyon commented 1 year ago

And also this:

require.resolve('@material/material-color-utilities')

Gives:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /web/workspace/node_modules/@material/material-color-utilities/package.json
flyon commented 1 year ago

So both these issues are because my project is not ESM.

I've sorted the [ERR_PACKAGE_PATH_NOT_EXPORTED] error out by not using require.resolve

Then for this issue: Package path . is not exported from package.. I've first switched to using dynamic imports. Because both node.js and webpack do support importing ESM modules in CJS projects with dynamic imports.

HOWEVER, I had to add "require" to the "exports" field in package.json of this library for the import to work:

  "exports": {
    ".": {
      "types": "./index.d.ts",
      "import": "./index.js",
      "require" : "./index.js"
    }
  },

With that in place it runs perfectly on node.js and in the browser. I'm getting color schemes etc.

So I suggest adding "require" to the exports Because even though this a ESM module, it can still work in CJS setups with a "require" export + dynamic imports.

Here's the code that works for me:

import('@material/material-color-utilities').then(({argbFromHex,themeFromSourceColor,applyTheme}) => {
    // Get the theme from a hex color
    const theme = themeFromSourceColor(argbFromHex('#f82506'));

    // Print out the theme as JSON
    console.log(JSON.stringify(theme,null,2));
});
WillsterJohnson commented 1 year ago

@flyon It might be worth opening a feature request for this in a new issue and referencing your comments here. I'm not sure if it's deliberate or coincidental that this package doesn't properly support the older CJS module style.

That said though, if you're able to use ESM in your project it's generally recommended, as ESM was made partially to fix several problems & difficulties with CJS.


@rodydavis should this issue be closed? AFAIK the original issue is fixed, though if this CJS problem is related enough maybe this should stay open.

rodydavis commented 1 year ago

Yeah I think it should be closed. Recommend to move to ESM and we could add the "require" to the exports if needed still.

WillsterJohnson commented 1 year ago

flyon opened #94 so this can be closed

rodydavis commented 1 year ago

I will see what we can do. Want to have first class ESM support