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

Allow ESM files to be used in Node.js #10479

Merged
merged 5 commits into from Jul 30, 2022
Merged

Allow ESM files to be used in Node.js #10479

merged 5 commits into from Jul 30, 2022

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jul 14, 2022

This is a bug fix to better allow using Chart.js in Node.js. Node.js will only load ESM files if they have an .mjs extension or the package has "type": "module". The latter would be a breaking change. However, this change should be invisible to users (unless they are doing import Chart from 'Chart.js/dist/chart.esm.js', which is undocumented and I would argue unsupported).

@LeeLenaleee
Copy link
Collaborator

LeeLenaleee commented Jul 15, 2022

Since the lib is on CDN's like JSDeliver which has the files for .esm.js also I think this will count as a breaking change too since they wont update anymore if you use https://cdn.jsdelivr.net/npm/chart.js@latest/dist/chart.esm.js

In 3.8.0 there is no information since its not in the top 5 requested resources but if you look at the 3.7.1 details you can see 8559 times this resource has been pulled this way:
image

@benmccann
Copy link
Contributor Author

We could include the file twice - once with .esm.js and once with .mjs extension if it's something that's a big concern.

@kurkle
Copy link
Member

kurkle commented Jul 18, 2022

Would be safer to have the files with both names. The auto path should also be updated.

etimberg
etimberg previously approved these changes Jul 20, 2022
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The *.mjs and *.mts need to be added to filed section of the main package.json to be included in the releases.

@benmccann
Copy link
Contributor Author

thanks. good catch! I've just updated that

@dangreen
Copy link
Collaborator

@benmccann @LeeLenaleee @kurkle @etimberg Hi. How about just add "type": "module" and "exports" fields to package.json? Then there no need to change file names and extensions.

@kurkle
Copy link
Member

kurkle commented Jul 28, 2022

@dangreen that sounds like better option, avoiding duplicate files. Can be done for v4.

@dangreen dangreen mentioned this pull request Jul 28, 2022
@benmccann
Copy link
Contributor Author

It's definitely the better long-term option. But I was assuming v4 isn't on the horizon. This change can be done in a backwards-compatible manner in the meantime until v4 is started. Are there any plans for v4?

@kurkle
Copy link
Member

kurkle commented Jul 28, 2022

Yes, v4 is planned after 3.9 is out.

@kurkle
Copy link
Member

kurkle commented Jul 28, 2022

I'd be ok going with this in 3.9 and going for type=module in v4.

@etimberg etimberg merged commit 6feb48b into chartjs:master Jul 30, 2022
@mlandalv
Copy link

mlandalv commented Aug 3, 2022

Hello! I don't think this PR works as intended.

Issue 1
module in package.json is not the entry for ESM [1]. Bundlers use it though, so for example webpack goes into ESM mode. Node crashes.

The tests were done with chart.js@3.9.0 and chartjs-plugin-datalabels@2.0.0.

// test.mjs
import ChartDataLabels from 'chartjs-plugin-datalabels';
import { Chart } from 'chart.js';

Chart.register(ChartDataLabels);
> node test.mjs

file:///mnt/arnold/dev/chartjs-tests/test.mjs:2
import { Chart } from 'chart.js';
         ^^^^^
SyntaxError: Named export 'Chart' not found. The requested module 'chart.js' 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 'chart.js';
const { Chart } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:189:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:541:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Node.js v18.7.0

Issue 2
helpers/helpers.js was renamed to .mjs but the export path in that file was not changed, so it doesn't fulfill the requirement of mandatory file extensions [2]. This makes (at least) webpack crash, which probably is the cause of issue #10547. Luckily that fix is easy.

ERROR in ../../node_modules/chart.js/helpers/helpers.mjs 1:0-36
Module not found: Error: Can't resolve '../dist/helpers.esm' in '/mnt/arnold/dev/inca/enkatregistret/node_modules/chart.js/helpers'
Did you mean 'helpers.esm.js'?
BREAKING CHANGE: The request '../dist/helpers.esm' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.

The solution so node can consume ESM is to use the exports field, which you discussed earlier. However, the risk is that it will break more things than it will solve. I'm not sure about the exact syntax, but something like:

  "exports": {
    ".": {
      "import": "./dist/chart.mjs",
      "require": "./dist/chart.js",
      "types": "./types/index.esm.d.ts"
    },
    "./helpers": {
      "import": "./helpers/helpers.mjs",
      "require": "./helpers/helpers.js",
      "types": "./helpers/helpers.mts"
    }
  }

I would also like to say a HUGE thanks to the awesome work put into this project. Big thanks! ❤️

[1] https://stackoverflow.com/a/42817320/1378453
[2] https://nodejs.org/api/esm.html#mandatory-file-extensions

@kurkle
Copy link
Member

kurkle commented Aug 3, 2022

Thanks @mlandalv for the details. I looked at issue 1 on Node and could not find a way to have commonjs and esm in the same package without exports defined. This however is also how Chart.js 3.8 works, so this can be fixed in v4 (with exports).

The 2nd issue needs to be fixed.

@benmccann
Copy link
Contributor Author

benmccann commented Aug 3, 2022

For issue 1, the idea is that you can reach into the package with a deep import. This isn't ideal from a user perspective, but at least unblocks users until 4.0 is available

Issue 2 has been fixed: #10552

@benmccann benmccann deleted the mjs branch August 3, 2022 15:11
inureyes added a commit to lablup/backend.ai-webui that referenced this pull request Aug 30, 2022
 * electron should be kept for cookie-based login support (will be
   replaced soon.)
 * chart.js 3.9 has rollup build problem with ESM support. see (
   chartjs/Chart.js#10599 , chartjs/Chart.js#10479 )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants