Skip to content

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

Merged
etimberg merged 5 commits into
chartjs:masterfrom
benmccann:mjs
Jul 30, 2022
Merged

Allow ESM files to be used in Node.js#10479
etimberg merged 5 commits into
chartjs:masterfrom
benmccann:mjs

Conversation

@benmccann

@benmccann benmccann commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

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

LeeLenaleee commented Jul 15, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

kurkle commented Jul 18, 2022

Copy link
Copy Markdown
Member

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

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

thanks. good catch! I've just updated that

@dangreen

Copy link
Copy Markdown
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

kurkle commented Jul 28, 2022

Copy link
Copy Markdown
Member

@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
Copy Markdown
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

kurkle commented Jul 28, 2022

Copy link
Copy Markdown
Member

Yes, v4 is planned after 3.9 is out.

@kurkle

kurkle commented Jul 28, 2022

Copy link
Copy Markdown
Member

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

mlandalv commented Aug 3, 2022

Copy link
Copy Markdown

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

kurkle commented Aug 3, 2022

Copy link
Copy Markdown
Member

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

benmccann commented Aug 3, 2022

Copy link
Copy Markdown
Contributor Author

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.

6 participants