ESM build, with helpers separated #7400

kurkle · 2020-05-22T08:28:24Z

This could be one approach

sgratzl

won't this results that the helpers are both in the esm bundle and separated? so they are still in the bundle but not accessible anymore.

Moreover, the helpers are just part of the issue. The bigger ones are the controllers and elements.

package.json

kurkle · 2020-05-22T08:52:49Z

won't this results that the helpers are both in the esm bundle and separated? so they are still in the bundle but not accessible anymore.

No, rollup is smart enough. Its referencing the helpers from the directory (and also creating a chunk for helpers.options to prevent circular dependency)

Here's the output:
esm_buid.zip

sgratzl · 2020-05-22T09:00:52Z

good to know. Another general question: what is the purpose of a minified esm build? who should reference it? I assume most users will use a bundler in an esm case so what are the benefits?

kurkle · 2020-05-22T09:04:38Z

good to know. Another general question: what is the purpose of a minified esm build? who should reference it? I assume most users will use a bundler in an esm case so what are the benefits?

no idea really. could be referenced directly from browser, i think?

sebiniemann · 2020-05-22T10:28:28Z

Is esm_buid.zip the final bundle? Asking cause it still consists of multiple files using ESM imports.

kurkle · 2020-05-22T10:33:32Z

Is esm_buid.zip the final bundle? Asking cause it still consists of multiple files using ESM imports.

Its the files that would be in npm package, excluding the UMD bundle. (and excluding the dist folder, that I later restored)

sebiniemann · 2020-05-22T12:01:28Z

That's nice 👍

Since this gives us something similar to what @sgratzl called the seperated case in #7371 (comment), is this a look at the future/3.x structure of ChartJS repo?

babel.config.json

benmccann · 2020-05-22T16:35:43Z

rollup.config.js

@@ -10,6 +10,22 @@ const terser = require('rollup-plugin-terser').terser;
 const pkg = require('./package.json');

 const input = 'src/index.js';
+const inputESM = {
+	'dist/chart.esm': 'src/index.esm.js',
+	'helpers/canvas': 'src/helpers/helpers.canvas.js',


Maybe we could programatically build the rollup input instead of having to list every file?

Though this is pretty explicit, so it would still be helpful to have a comment showing exactly what it's doing if done programatically

Is this necessary to do programatically on the long term?

If esm_buid.zip is not anticipating the future repo structure (which would cause this to become obsolete), I do not see the benefit to generate an additional multi-file ESM package, which basically restructures src/, instead of delivering src/ directly, as it already is a fully working ESM package.

The problem is, it is not a fully working esm module. Because there are proposal features used.

Oh I see, wasn't aware of this.

If it is in order to pull the source through babel, we could also preserve the current structure and wouldn't need to define a restructured package.

However, I am unsure if all this effort is worth it to build ChartJS on top of non-JS syntax/features. I assume that you already need to babel the source to apply ESLint (which only supports Stage 4) and your testing environment?

Is the only used proposal feature the out of constructor-class property initialisation from @babel/plugin-proposal-class-properties? Object.assign and static methods should be Stage 4 since 2015? If we only need to move the property initialisation into the class constructors (and static ones in the module scope), in order to avoid all this additional tooling, building (and the whole maintenance behind it), I believe it would be worth it 😃

Yeah, it's just the static initializations. It wouldn't be hard to change. I wasn't fully aware of the implications when I originally introduced it and would definitely be in favor of removing it and babel from the ES builds

We still need the tooling for IE support, so it does not really reduce any maintenance. But I created a request for moving the statics to module scope: #7425

Wouldn't it remove the need to babel everything during testing and linting (as well as the dependencies necessary for this)?

Since IE does not support the ESM syntax, I assume that at most IE-compatible packages need Babel after their generation?

I would also suggest to never apply Babel to the ESM package. This would allow users to downgrade the JS standard of ChartJS (which can have quite the performance impact if you simulate some native functions) based on their own needs.

Maybe we could programatically build the rollup input instead of having to list every file?

Added glob for that.

@sebiniemann Chart.js v3 umd bundle is IE compatible and we need babel for that. Its produced from the same source. Some tests use ESM syntax, so we need to rollup those so karma can cope with them. We also need babel for the tests to get coverage right (I've tried to get it done with rollup-plugin-istanbul and several other packages, but it just does not work right).

I am 100% with you on this :)

Not sure why babel is needed for test coverage, might be some special configuration. We could also remove babel-eslint (hard to see, as it comes as a sub-dep of eslint-config-esnext).

Babel will surly remain in most projects to deliver at least one fast-to-use version with guaranteed browser compatibility (which is nice), but as testing and linting is not bound to use land, it shouldn't be necessary for these.

benmccann · 2020-05-22T17:03:27Z

It might be confusing that we have Line and line in the final output representing different things. Maybe we should leave it as LineController instead of renaming to line?

I also wonder about the names of the plugins

kurkle · 2020-06-09T09:23:25Z

It might be confusing that we have Line and line in the final output representing different things. Maybe we should leave it as LineController instead of renaming to line?

I also wonder about the names of the plugins

These are related to the registration and should be handled in #7435

kurkle · 2020-06-09T09:29:38Z

This makes the helpers fully shakeable. Core stuff is not yet, because of the registrations etc. But the exports are fine, just need to remove the internal access.

etimberg · 2020-06-09T11:54:54Z

Looks good to me. I'll play around with this locally tonight and test it out.

babel.config.json

benmccann · 2020-06-09T15:34:38Z

I was trying to remember the high-level idea here since it's been a few weeks since we were really active on this stuff. Here's a link to the original discussion: #7371 (comment)

etimberg

I played around with this a bit locally and it seemed ok. npm pack --dry-run reports the helpers being included.

benmccann

this looks good to me. just one super minor issue I noticed

a couple things about the documentation I was thinking, which could be handled in a follow-up if we wanted:

we should probably have the TypeDoc be generated off the es 6 output bundle or else the namespaces won't match what users see
we should probably update https://www.chartjs.org/docs/master/developers/charts to show it using es6 + npm way of doing things

rollup.config.js

sebiniemann · 2020-06-18T19:17:05Z

If not already done by this, is it possible to also deliver src/*? Since we use ChartJS over npm and our own rollup process to generate our front-end, there is no actual need for a pre-chunked ESM version.

Its actually how we consume most of our dependencies and how its specified in their package.json (like the aws-sdk for example)

kurkle · 2020-06-18T19:48:23Z

Aws-sdk is a type script project, you are consuming the *.ts?
Oh, it might be a mix of things actually, nvm.

sebiniemann · 2020-06-18T20:04:24Z

aws-sdk is mostly written in JS (with CommonJS module flavor) and uses *.d.ts files to provide typescript type information.

kurkle · 2020-06-18T21:06:20Z

Since we use ChartJS over npm and our own rollup process to generate our front-end, there is no actual need for a pre-chunked ESM version.

What is the down side of processing the distributed files? Its more likely compatible with webpack etc, because rollup fills the missing '.js' to imports etc, that we really don't care to manually keep track with.

Only thing that I can think of you gaining from direct source npm package, is the line numbers for debugging. But that is going to be in the tag anyway, not the master branch.

Previously our source was not usable directly. That day might come again, and if it was directly published. it would be a breaking change. We don't want to tie our source tree down.

Another thing to consider is that its only your use case we are talking about here. There are quite a few other use cases out there, that would need to be considered. At the minimum those .js extensions would need to be added to imports. I bet there are other things as well, we are doing "wrong" for some unknown bundler out there.

This works, it should not really harm your use case, and I really don't see a valid reason (in Chart.js perspective!) to change from dist to src.

sebiniemann · 2020-06-18T23:55:48Z

That's totally fine for me if you are not considering this at the moment 😃

Since we have our own rollup process, it will work for us either way -- regardless which file structure is shipped. It was more meant to say that a pre-chunked version might not be necessary for npm+ webpack/rollup users (might be similar for CDN users, as I believe that single file libraries are more typically consumed).

Regarding your questions, there are only two main reasons I see for this:

The source folder already is a multi file esm package. Generating another one from it seems unnecessary, beside adding additional build steps/configurations.
As you said before, source files/line numbers match better in buggy situations, which helps users to match the shipped source with Github's source (more beneficial in very large, complex projects) and eases project contributions.

There is also a special case about caching, when ChartJS is consumed by <script type="module" ..> (regarding how much bandwidth is used to acquire an updated ChartJS version), but I don't believe that this has some meaningful weight for this discussion 😅

In summary I would say that the ChartJS source is already written in JS and shouldn't need a build step to be usable. And only consider build steps to provide additional packages (umd package, single-file-packages, pre-babel builds, ...).

Regarding the missing file extension, I personally consider the omission to be a mistake/bug, since something like something like the resolution of module file extensions does not seem to be a necessary part of the ECMAScript standard. It is not very likely to change, as browsers would require multiple server round-trips/HTTP request to support this.

If you like to address this, you can ensure that it is always set via the import/extensions rule from eslint-plugin-import

We don't want to tie our source tree down.

I am not sure what you meant by this?

kurkle · 2020-06-19T03:26:00Z

I am not sure what you meant by this?

Just that we want to produced and publish ECMA script compatible package, but might want to use typescript in parts of the source (for example). Or anything other that transpile to valid package.
We might want to use proposal features for cleaner source.

benmccann · 2020-06-23T23:00:20Z

I agree it'd be nice not to have a build step since we already have the source code in ES6. But that would mean that end users need to import one component per line instead of being able to do import { Chart, LineController, TimeScale, LogarithmicScale } from 'chart.js';. It does make it slightly trickier for people who want to contribute to Chart.js, but I think the tradeoff is worth it to make a better experience using the library. (The other alternative would be to put all these classes in the same root directory in the source tree to match the output here, but that would be less organized and make it hard to use git history, so I don't think that's worth it either)

* ESM build, with helpers separated * Remove umd environment * Include the chunks in package

kurkle mentioned this pull request May 22, 2020

v3: consider changing export logic for esm modules #7371

Closed

sgratzl reviewed May 22, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

kurkle mentioned this pull request May 22, 2020

Toolchain #7402

Merged

benmccann reviewed May 22, 2020

View reviewed changes

babel.config.json Outdated Show resolved Hide resolved

benmccann reviewed May 22, 2020

View reviewed changes

kurkle force-pushed the esm-build branch from d2c3107 to 1531817 Compare June 9, 2020 06:14

ESM build, with helpers separated

ddab2e5

kurkle force-pushed the esm-build branch from 1531817 to ddab2e5 Compare June 9, 2020 06:56

kurkle marked this pull request as ready for review June 9, 2020 07:01

Fix coverage

ae0770c

benmccann reviewed Jun 9, 2020

View reviewed changes

babel.config.json Outdated Show resolved Hide resolved

kurkle mentioned this pull request Jun 9, 2020

Tree shakeable ESM build & fully functional #7485

Closed

kurkle added 2 commits June 9, 2020 21:33

Remove umd environment

f397ed5

Restore babel.config.json

f90bb83

etimberg previously approved these changes Jun 14, 2020

View reviewed changes

benmccann reviewed Jun 15, 2020

View reviewed changes

rollup.config.js Show resolved Hide resolved

kurkle added the type: infrastructure label Jun 17, 2020

kurkle dismissed etimberg’s stale review via 76f3087 June 18, 2020 10:56

kurkle force-pushed the esm-build branch from 76f3087 to f90bb83 Compare June 18, 2020 11:02

Include the chunks in package

f1be344

benmccann approved these changes Jun 18, 2020

View reviewed changes

etimberg approved these changes Jun 18, 2020

View reviewed changes

etimberg merged commit 40e9029 into chartjs:master Jun 23, 2020

etimberg pushed a commit that referenced this pull request Sep 1, 2020

ESM build, with helpers separated (#7400)

5a08594

* ESM build, with helpers separated * Remove umd environment * Include the chunks in package

kurkle deleted the esm-build branch December 31, 2020 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESM build, with helpers separated #7400

ESM build, with helpers separated #7400

kurkle commented May 22, 2020 •

edited

Loading

sgratzl left a comment

kurkle commented May 22, 2020

sgratzl commented May 22, 2020

kurkle commented May 22, 2020

sebiniemann commented May 22, 2020

kurkle commented May 22, 2020 •

edited

Loading

sebiniemann commented May 22, 2020 •

edited

Loading

benmccann May 22, 2020

sebiniemann May 22, 2020 •

edited

Loading

kurkle May 22, 2020

sebiniemann May 22, 2020 •

edited

Loading

benmccann May 22, 2020

kurkle May 26, 2020

sebiniemann Jun 2, 2020 •

edited

Loading

kurkle Jun 9, 2020

kurkle Jun 9, 2020

sebiniemann Jun 18, 2020

benmccann commented May 22, 2020 •

edited

Loading

kurkle commented Jun 9, 2020

kurkle commented Jun 9, 2020

etimberg commented Jun 9, 2020

benmccann commented Jun 9, 2020

etimberg left a comment

benmccann left a comment

sebiniemann commented Jun 18, 2020

kurkle commented Jun 18, 2020 •

edited

Loading

sebiniemann commented Jun 18, 2020 •

edited

Loading

kurkle commented Jun 18, 2020 •

edited

Loading

sebiniemann commented Jun 18, 2020 •

edited

Loading

kurkle commented Jun 19, 2020

benmccann commented Jun 23, 2020

ESM build, with helpers separated #7400

ESM build, with helpers separated #7400

Conversation

kurkle commented May 22, 2020 • edited Loading

sgratzl left a comment

Choose a reason for hiding this comment

kurkle commented May 22, 2020

sgratzl commented May 22, 2020

kurkle commented May 22, 2020

sebiniemann commented May 22, 2020

kurkle commented May 22, 2020 • edited Loading

sebiniemann commented May 22, 2020 • edited Loading

benmccann May 22, 2020

Choose a reason for hiding this comment

sebiniemann May 22, 2020 • edited Loading

Choose a reason for hiding this comment

kurkle May 22, 2020

Choose a reason for hiding this comment

sebiniemann May 22, 2020 • edited Loading

Choose a reason for hiding this comment

benmccann May 22, 2020

Choose a reason for hiding this comment

kurkle May 26, 2020

Choose a reason for hiding this comment

sebiniemann Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

kurkle Jun 9, 2020

Choose a reason for hiding this comment

kurkle Jun 9, 2020

Choose a reason for hiding this comment

sebiniemann Jun 18, 2020

Choose a reason for hiding this comment

benmccann commented May 22, 2020 • edited Loading

kurkle commented Jun 9, 2020

kurkle commented Jun 9, 2020

etimberg commented Jun 9, 2020

benmccann commented Jun 9, 2020

etimberg left a comment

Choose a reason for hiding this comment

benmccann left a comment

Choose a reason for hiding this comment

sebiniemann commented Jun 18, 2020

kurkle commented Jun 18, 2020 • edited Loading

sebiniemann commented Jun 18, 2020 • edited Loading

kurkle commented Jun 18, 2020 • edited Loading

sebiniemann commented Jun 18, 2020 • edited Loading

kurkle commented Jun 19, 2020

benmccann commented Jun 23, 2020

kurkle commented May 22, 2020 •

edited

Loading

kurkle commented May 22, 2020 •

edited

Loading

sebiniemann commented May 22, 2020 •

edited

Loading

sebiniemann May 22, 2020 •

edited

Loading

sebiniemann May 22, 2020 •

edited

Loading

sebiniemann Jun 2, 2020 •

edited

Loading

benmccann commented May 22, 2020 •

edited

Loading

kurkle commented Jun 18, 2020 •

edited

Loading

sebiniemann commented Jun 18, 2020 •

edited

Loading

kurkle commented Jun 18, 2020 •

edited

Loading

sebiniemann commented Jun 18, 2020 •

edited

Loading