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

Add package.json exports property #9597

Closed
wants to merge 1 commit into from
Closed

Conversation

gavinr
Copy link

@gavinr gavinr commented Aug 27, 2021

This allows modern build systems (vite) to properly use the correct module type (ESM).

When this is not present, when using Chart.js in Sveltekit, when running a build it will give an error Named export 'Chart' not found. The requested module 'chart.js' is a CommonJS module, which may not support all module.exports as named exports.

More info: https://medium.com/swlh/npm-new-package-json-exports-field-1a7d1f489ccf
Also, for background: sveltejs/kit#1498 (comment) and sveltejs/kit#928 (comment)

@gavinr
Copy link
Author

gavinr commented Aug 30, 2021

@benmccann does this resolve #9436 ?

"type": "module",
"exports": {
"import": "./dist/chart.esm.js",
"require": "./dist/chart.js"
Copy link
Contributor

@benmccann benmccann Aug 30, 2021

Choose a reason for hiding this comment

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

You'd also have to export package.json. Some build tools such as rollup-plugin-svelte will try to consume it. I'm not sure if this is exactly the right syntax, but I think you could do it with:

    ".": {
      "import": "./dist/chart.esm.js",
      "require": "./dist/chart.js",
    }
    "./package.json": "./package.json"

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing helpers and auto need to be added too. And the type should not be changed, at least not before v4.

@@ -4,6 +4,11 @@
"description": "Simple HTML5 charts using the canvas element.",
"version": "3.5.1",
"license": "MIT",
"type": "module",
Copy link
Contributor

Choose a reason for hiding this comment

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

do you have to add "type": "module"? I expect it might be a breaking change

Copy link
Contributor

Choose a reason for hiding this comment

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

the tests are failing and I expect this line is why

@benmccann
Copy link
Contributor

I haven't tested, but yes, I imagine it would fix that issue

@kurkle
Copy link
Member

kurkle commented Sep 2, 2021

Discussion in #9405 is related (also has a workaround for vite).

@gavinr
Copy link
Author

gavinr commented Sep 14, 2021

I worked around this issue by moving my Chart.register(...registerables); line into a place later in my code that gets created after the DOM: gavinr/stl-lambert-airport-data@b029b23

I'll close this PR for now since I don't have enough chartjs knowledge to push it forward, but I hope it can serve as inspiration for a future PR. Thanks!

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 this pull request may close these issues.

3 participants