Skip to content

Conversation

@dangreen
Copy link
Collaborator

Alternative of #10479

More changes than expected, but still.

To import CommonJS modules in/from ESM project .cjs extension is required.

@kurkle kurkle added this to the Version 4.0 milestone Jul 28, 2022
@dangreen
Copy link
Collaborator Author

dangreen commented Jul 29, 2022

I think this PR also resolves this issue #9436

If we still want to support commonjs, cjs bundle from this PR has compatible with ESM exports:

Chart.helpers = {...helpers};
Chart._adapters = _adapters;
Chart.Animation = Animation;
Chart.Animations = Animations;
Chart.animator = animator;
Chart.controllers = registry.controllers.items;
Chart.DatasetController = DatasetController;
Chart.Element = Element;
Chart.elements = elements;
Chart.Interaction = Interaction;
Chart.layouts = layouts;
Chart.platforms = platforms;
Chart.Scale = Scale;
Chart.Ticks = Ticks;
Object.assign(Chart, controllers, scales, elements, plugins, platforms);
Chart.Chart = Chart;
module.exports = Chart;

@benmccann What do you think?

@benmccann
Copy link
Contributor

I'd love to drop CJS and do ESM-only if possible

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.

I'm not sure how the files should be named, because I guess chart.js can't be UMD when type='module'.

@dangreen dangreen requested review from benmccann and kurkle and removed request for benmccann August 3, 2022 07:17
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.

Just one more comment (considering all of the package.json's)

kurkle
kurkle previously approved these changes Aug 3, 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.

Needs a rebase though

@dangreen dangreen requested review from benmccann and kurkle and removed request for benmccann August 3, 2022 16:06
kurkle
kurkle previously approved these changes Aug 3, 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.

Something about this should be mentioned in https://www.chartjs.org/docs/master/migration/v4-migration.html but I don't want to hold because of that.

@dangreen dangreen dismissed stale reviews from kurkle, etimberg, and benmccann via 606b9be August 4, 2022 09:30
@dangreen dangreen mentioned this pull request Aug 4, 2022
etimberg
etimberg previously approved these changes Aug 4, 2022
kurkle
kurkle previously approved these changes Aug 4, 2022
@dangreen
Copy link
Collaborator Author

dangreen commented Aug 4, 2022

I will resolve conflicts later today

@dangreen dangreen dismissed stale reviews from kurkle and etimberg via cfd8613 August 4, 2022 13:45
@benmccann
Copy link
Contributor

Should we keep the unminified UMD build? It might help people if they encounter an error in the minified build or otherwise want to debug. Perhaps we should have a chart.umd.js and chart.umd.min.js?

@dangreen
Copy link
Collaborator Author

dangreen commented Aug 4, 2022

@benmccann maybe just emit sourcemaps for UMD bundle ?

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.

4 participants