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

Chart.js does not import as es6 module #5179

Closed
backspaces opened this issue Jan 23, 2018 · 46 comments · Fixed by #6619
Closed

Chart.js does not import as es6 module #5179

backspaces opened this issue Jan 23, 2018 · 46 comments · Fixed by #6619

Comments

@backspaces
Copy link

The instructions on http://www.chartjs.org/docs/latest/getting-started/integration.html show chart.js being imported as an es6 module:
snap 01 23 18-15 19 58

When I use this import statement within a module using chrome latest, it fails:

import Chart from 'https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.1/Chart.js'

The error is:

Uncaught SyntaxError: The requested module does not provide an export named 'default'

Expected Behavior

I think this is a bug, the docs say it should work. However, looking at the code, I see no es6 export.

Current Behavior

It fails with the above error.

Possible Solution

You might try converting to an es6 build using rollup for all conversions to other modules.
https://medium.com/@backspaces/es6-modules-part-1-migration-strategy-a48de0b7f112

Steps to Reproduce (for bugs)

  1. Within a browser supporting modules, create a <script type="module">
  2. Inside that, import Chart.
  3. The error will occur.
<html>
<head>
  <title>test</title>
</head>
<body>
  <script type="module">
    import Chart from 'https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.1/Chart.js'

    console.log(Chart)
  </script>
</body>
</html>

Context

I am using chart.js to visualize results of agent based models. The repos are:

Environment

  • Chart.js version: latest .. 2.7.1
  • Browser name and version: chrome 63.0.3239.132
  • Link to your project: Above.
@etimberg
Copy link
Member

@simonbrunel any thoughts? I know this got tested at one point

@simonbrunel
Copy link
Member

We currently use browserify to generate UMD but we didn't upgrade for a long time so maybe there is an easy fix that doesn't break any integration?

I don't know from where this documentation about ES6 import compatibility comes from but are UMD builds compatible with es6 import since it doesn't provide a default export? For example, UMD generated by rollup: I'm not sure it works because there is no default export. That might be an extra step / option to enable es6 import support?

(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
	typeof define === 'function' && define.amd ? define([], factory) :
	(global['Chart'] = factory());
}(this, (function () {
  // ...
  return Chart;
})

#4478 is about migrating to rollup but this might not happen until v3 because it could break node.js integrations.

@backspaces
Copy link
Author

The huge advantage of converting the code to es6 modules is that then, using rollup and possibly babel (for es5) is that you can then convert to every format. It should not impact your node compatibility because rollup easily converts to commonJS.

It also lets you rollup subsets of your project, which support fewer chart types.

Can you say just what node problems you have?

@simonbrunel
Copy link
Member

We don't have any problem with node :) We care to not break projects that use Chart.js and I'm pretty sure some people integrated it using var Chart = require('./node_modules/chart.js/src/chart.js'), which would break if we switch to es6 exports (src/chart.js being an es6 module in that case). That's why it would need to wait until the next major version.

I agree that migrating to rollup/es6 modules is a good thing but we first need to refactor/clean our way to use modules and that's what we started with #4478 (we already use rollup in other projects such as chartjs-plugin-datalabels and chartjs-plugin-deferred).

Though, I'm not sure UMD builds can be imported via es6 import? That means your way to include Chart.js from our UMD build (https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.1/Chart.js) may not work, even with rollup and a code initially based on es6 modules.

@backspaces
Copy link
Author

I hear ya! One solution Three.js used was to keep the old names the same, and using an es6 rollup called three.module.js. The older three.js name was kept for the older code.

The source itself obviously changed quite a bit but they did not make the source part of the dist/.

So you could do likewise: convert to es6 source files, rollup to a cjs bundle named chart.js.

OTOH, waiting for the 3.0 build makes sense in terms of project management.

I think I can convert chart.js to modules using this stunt:

https://medium.com/@backspaces/es6-modules-part-2-libs-wrap-em-up-8715e116d690

so no worries.

@backspaces
Copy link
Author

But lets change the docs, others will make the same mistake I did.

@benmccann
Copy link
Contributor

This was added to the docs in:
eb2b04a

And then updated in:
696f8d3

I wonder if something was done to break it at some point

@backspaces
Copy link
Author

I now seriously need plotting for a project I'm involved in .. and we're es6 module based.

Our code is vanilla JS es6 w/ import/export. Our very simple workflow uses rollup to create two bundles: es5/UMD and es6/ESM. We use unpkg to serve both formats:

The latter uses the module: property in package.json.

Could you think about creating such a dual-release? I see that you use "require", thus browserify. I know very little about that tooling, but I bet you can find tools that can create ESM from this.

If you look at d3, you'll see they create bundles for both that work well on unpkg.com.

Admittedly you may prefer to simply convert your source to es6 modules, and then bundle to a UMD compatible with your existing chart.js (which works in both the browser and node) and an ESM that is fine for es6 oriented repos. You'll be surprised how much easier it is to start with your core as ESM and convert to any format you'd like.

@benmccann
Copy link
Contributor

@backspaces I think the issue might be that you're using Chart.js 2.7.1 and looking at the docs for Chart.js 2.7.2. Can you try upgrading to Chart.js 2.7.2?

I have a vue.js + webpack app that's using Chart.js via ES6 import as described in the docs and it's working for me:

import Chart from 'chart.js';
var chart = new Chart(ctx, cfg);

@backspaces
Copy link
Author

Hi, thanks for the reply.

Here's the problm: Your import above is not the es6 module syntax which needs a "path" like ./chart.js. Your workflow is making adjustments to your code, I think. Webpack likely is doing it.

I tried changing the html in the OP to 2.7.2 and still receive the error.

import Chart from 'https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.2/Chart.js'

.. gives:

Uncaught SyntaxError: The requested module 'https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.2/Chart.js' does not provide an export named 'default'

And looking at the code, it does not appear to have any es6 export.

Using unpkg to view the repo, https://unpkg.com/chart.js@2.7.2/ (dashboard view)
.. I could not find a module: field in package.json, nor an es6 export in the code.
And using the https://unpkg.com/chart.js@2.7.2?module syntax gets an error.

Asking on Stackoverflow, I did get an interesting response tho: using a "bare" import worked!

 import 'https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.2/Chart.bundle.min.js'

There's a working example: https://stackoverflow.com/questions/48410525/how-do-i-import-chartjs-as-an-es6-module

I have no idea how that works! It ends up working like a script tag, putting Chart in window. I only tried chrome.

@benmccann
Copy link
Contributor

The difference could also be that I'm using NPM and you're importing a pre-built script. It'd be really interesting to try NPM without webpack to understand whether webpack is involved.

@Jareechang do you have thoughts here? (asking because you documented and might have some insight into how this works)

@Jareechang
Copy link
Contributor

hey @benmccann, my changes were related to #3788 as it was breaking the import. Unfortunately, I do not have much insights into how this works 😞. I did also use webpack when I was testing it.

However, It does sound like es6 is not supported atm as @simonbrunel has mentioned that the UMD builds do not provide a default.

@backspaces
Copy link
Author

Just a note: You can't create a single file that is both <script>able/require-able(node cjs) and es6 importable.

The current favorite approach is to have the src/*.js code written as individual es6 modules, then bundled, w/ Rollup, to a UMD (script/require) and to a separate ESM bundle. Then place a pointer to the ESM in the package.json under the "module:" property.

This is even made simpler with unpkg which recognizes the ?module query string as a request for the module, via package.json.

It is a shame that es6 modules were so difficult to get started. Huge workflow, tools like browserify, jspm, and webpack, typescript having their own modules for quite a while, and so on. And now it's hard for us to go back to the simplicity that modern browsers have made possible. Sigh. :)

@lpellegr
Copy link
Contributor

I am facing the same issue. Now that ES6 imports are natively supported in all major browsers it would be nice to have a solution for Chart.js.

@benmccann
Copy link
Contributor

benmccann commented Jul 29, 2018

Docs updated in #5555

@benmccann benmccann added this to the Version 2.8 milestone Jul 29, 2018
@lpellegr
Copy link
Contributor

lpellegr commented Jul 29, 2018

@benmccann You closed the issue but it seems not fixed. Version 2.7.2 is not providing an ESM version of the library. The original issue is not related to using Webpack only or not. Some packaging methods, such as Polymer 3 apps with polymer-cli handle all build steps and you have no control over them (this is my use case). In that case, you need an ESM version of the library you want to import to get bundles generated and optimized.

I just noticed you tag this issue for milestone Version 2.8. Are you going to provide ESM flavors with NPM packages starting with version 2.8.x?

@benmccann
Copy link
Contributor

We updated the docs, which were incorrect, to clarify that chart.js does not yet have es6 support. I don't think that es6 support will be added until 3.0

@backspaces
Copy link
Author

When will 3.0 be released? Is there a beta I could try?

@benmccann
Copy link
Contributor

benmccann commented Aug 8, 2018

There are no plans to start working on 3.0 at the moment

@backspaces
Copy link
Author

Just a quick update: I believe a simple rollup, along with a commonJS plugin, produces a usable es6 module. The Rollup looks like:

import commonjs from 'rollup-plugin-commonjs'
import nodeResolve from 'rollup-plugin-node-resolve'
export default {
    input: './node_modules/chart.js/dist/Chart.js',
    output: {
        file: 'chart.esm.js',
        format: 'esm',
    },
    plugins: [
        nodeResolve(),
        commonjs(),
    ],
}

This is built using the chart.js npm install. You'll also have to node install rollup-plugin-commonjs and rollup-plugin-node-resolve

I don't have a test env for this but here's the output: http://backspaces.net/temp/chart.esm.js
I've built a trivial script to make sure the browser imports it correctly (yes browsers do this!):

<script type="module">
    import chart from './chart.esm.js' // default export
    console.log(chart)
</script>

If anyone uses this and it works, let us know. We also could just add to the projects workflow if it is considered useful.

Here's the SO discussing this: https://stackoverflow.com/questions/52068933/can-rollup-plugins-convert-the-majority-of-legacy-libraries-to-es6-modules/52076713#52076713

@simonbrunel simonbrunel removed this from the Version 2.7.3 milestone Dec 7, 2018
@simonbrunel simonbrunel reopened this Dec 7, 2018
@lpellegr
Copy link
Contributor

lpellegr commented Dec 7, 2018

@simonbrunel Thanks for your answer and pointing the wrapper. I have adapted the rollup configuration to use the original source file:

https://github.com/noticeableapp/Chart.js/commit/466fc8087d354dd39aa127f365b5ec86003dc3a6

@lpellegr If I understand correctly, we need a separate .esm.js file to be loadable as an ES module because this can't be handled by the UMD build?

Yes, that's the idea.

We are progressively migrating our code to be modular (#4478) and I was planning to migrate all of our builds to use rollup. I'm not fan of mixing technologies (rollup + browserify) and increase dependencies, so would prefer to wait until we switch to rollup.

What is proposed is far from perfect but the migration you mention, although fantastic, is a huge work that is open since more than a year. ESM is supported by modern browsers. ESM imports are becoming a first-class solution (e.g. it's really helpful with Polymer 3, lit-html, TypeScript).

The changes initially proposed by @backspaces could work for a transition period. Since they are non-breaking, you could even release them as 2.X.0. Later, when the code is modularized and only rollup used you could simply update the reference in package.json for module and still release as 2.Y.0, if the API is not changed.

Regarding the NPM package, it seems to include doc, samples and tests. It's not the additional ESM variant of the original source code that will make a big difference in terms of size.

@simonbrunel
Copy link
Member

In order to consider this transition period where we would mix browserify and rollup, I would like to investigate if we couldn't completely switch now to rollup without breaking changes. Here is a wip branch (rollup+esm) using rollup for all of our builds, including Chart.esm.js and Chart.esm.min.js, but also to run unit tests. It still needs lot of testing to confirm it doesn't break anything (I will need help for that) but it looks pretty good.

@backspaces @lpellegr the current setup generates the ESM builds without moment. I'm not familiar with ES modules "in the browser" and don't know how peer dependencies should be handled:

<script type="module">
    import chart from './dist/Chart.esm.js';
  
    // throws...
    // Uncaught TypeError: Failed to resolve module specifier "moment". Relative references must start with either "/", "./", or "../".
</script>

Any idea?

@backspaces
Copy link
Author

Moment is now natively in es6, within src/
https://github.com/moment/moment#port-to-ecmascript-6-version-2100
https://github.com/moment/moment/blob/develop/src/moment.js

The repo exports src/ and the package.json has an es6 module header, thus this works:
https://unpkg.com/moment?module

So you're close!

@simonbrunel
Copy link
Member

@backspaces I already figured out that moment was ES6 ready, my question is about your <script> snippet: how to make Chart.esm.js to consume https://unpkg.com/moment?module, the fist line of Chart.esm.js being:

import moment from 'moment';

which fail with:

Uncaught TypeError: Failed to resolve module specifier "moment". Relative references must start with either "/", "./", or "../".

We will also need to find a way to make moment (and later luxon - or whatever) optional for the ESM build.

@benmccann
Copy link
Contributor

The build has now been migrated to rollup: #5904

@wbern
Copy link

wbern commented Mar 13, 2019

Is there a reason why the main field doesn't just point to the dist directory? I don't want to install moment in my Chart.js-consuming package, so I'd rather get the transpiled product. I thought this was standard behavior.

For reference, I'm using pnpm which keeps good track of these inconsistencies: pnpm/pnpm#1700

@simonbrunel
Copy link
Member

@wbern that will be the case in the upcoming v2.8.0.

@wbern
Copy link

wbern commented Mar 14, 2019

For the record, chart.js works with pnpm without issues, I had a small mistake in my config (in webpack, symlinks: false doesn't work well with pnpm)

@simonbrunel simonbrunel mentioned this issue Apr 2, 2019
3 tasks
@krumware
Copy link

krumware commented Apr 3, 2019

Per the reference to

@wbern that will be the case in the upcoming v2.8.0.

Per this update, should we expect to no longer receive the following:

Uncaught SyntaxError: The requested module './node_modules/chart.js/dist/Chart.js' does not provide an export named 'default'

when using import Chart from 'chart.js'?
We are still seeing this error, but the updates here make it seem like we shouldn't be.

Thanks!

@derhuebiii
Copy link

Per the reference to

@wbern that will be the case in the upcoming v2.8.0.

Per this update, should we expect to no longer receive the following:

Uncaught SyntaxError: The requested module './node_modules/chart.js/dist/Chart.js' does not provide an export named 'default'

when using import Chart from 'chart.js'?
We are still seeing this error, but the updates here make it seem like we shouldn't be.

Thanks!

Same here, any updates on this?

@wbern
Copy link

wbern commented Jun 17, 2019

See my latest comment

@backspaces
Copy link
Author

Import needs you to use a complete path: import Chart from 'chart.js'? is not one.

So try import Chart from './path/to/chart.js'

@backspaces
Copy link
Author

backspaces commented Jun 19, 2019

Just a quick note. This: #5179 (comment) still works fine, and we're using it at pika/web FredKSchott/snowpack#62 to look into automatic conversion of CJS/UMD format to es6 modules.

Here's the current rollup:

import commonjs from 'rollup-plugin-commonjs'
import resolve from 'rollup-plugin-node-resolve'

export default {
    input: 'Chart.js',
    output: {
        file: 'chart.esm.js',
        format: 'esm',
    },
    plugins: [resolve(), commonjs()],
}

The test repo has these node dependencies:

├── chart.js@2.8.0
├── rollup@1.15.6
├── rollup-plugin-commonjs@10.0.0
└── rollup-plugin-node-resolve@5.0.3

If you'd like, I can make a github repo of the experiment, but it really is trivial.

Here's a link to a pika article if you're curious.

@backspaces
Copy link
Author

Is there, btw, a reason this is not already in the current chart.js? Make a rollup, add a "module": property to package.json? That doesn't seem very odd.

@lpellegr
Copy link
Contributor

lpellegr commented Sep 5, 2019

That's really sad. More than a year and a half, suggestions provided, no breaking changes, and we still need to struggle to manage case manually. What a shame.

@krumware
Copy link

krumware commented Sep 5, 2019

I have not yet tried it on chart.js specifically, but per @backspaces' comments above, and feature requests in pika-web, you can now do package install-time conversions of commonjs modules to es-modules using pika-web. It's worth a shot.
Extra info toward the end of this thread: FredKSchott/snowpack#62

@lpellegr
Copy link
Contributor

lpellegr commented Sep 5, 2019

@krumware Thanks but Chart.js is now building with Rollup and adding support for ESM is just a matter of adding a few lines of codes in package.json and the Rollup configuration. Would be great to know why they are not included by default?

I ended up by forking the release branch, made the small changes mentioned above:

https://github.com/ipregistry/Chart.js/commit/b981ac18715f94253a502efeb852b0c4d6218db9

and published my own package:

https://www.npmjs.com/package/@ipregistry/chart.js

@krumware
Copy link

krumware commented Sep 5, 2019

Thanks for the heads-up! great to know there are many options. Hopefully they pull your changes so we can get that esm dist out of the box if you potentially aren't able to maintain the fork long-term.

@benmccann
Copy link
Contributor

If anyone wants to send a PR to improve our rollup & package.json config that'd be great as long as it's backwards compatible with all the documented methods of using Chart.js

@benmccann
Copy link
Contributor

@lpellegr I took a look at your change. It looks like your es6 version bundles moment.js. Was that a conscious decision? I think my initial inclination would have been not to include it

@sebiniemann
Copy link
Contributor

sebiniemann commented Oct 27, 2019

I hope this pull request is as requested. Due to the old age of this ticket and its lively discussion, being in contrast with the small additions needed to the rollup config, I hoped I didn't overlooked some hidden/deeper problem.

At least, I tested the new builds in a pure browser scope, as well as rollup scope (without commonjs support). Both working as expected 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants