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

Correct auto.esm.js import to allow module to work in browser #10341

Merged
merged 1 commit into from May 11, 2022

Conversation

kylejonesdev
Copy link
Contributor

  • Added needed file extension to import statement
  • Remedied the browser console error referenced in Import in auto.esm.js is wrong #10303
  • Passed all tests
  • No example attached because no practical change in functionality

@etimberg etimberg requested a review from kurkle May 11, 2022 10:57
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Approving since the build worked. I'm surprised other spots did not need their imports changed

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.

The PR title should be updated to something descriptive.

The helpers imported from dist/chart.esm.js seem to have the .js extension already and everything else is in that file. So I'm confident this change is really enough.

I don't see any benefits in importing the module version that way though, non-minimized and only some helpers extracted to separate bundle. But maybe I'm just missing something.

(the chart.min.js is ~69kb while the chart.esm.js is ~370kb)

@etimberg etimberg linked an issue May 11, 2022 that may be closed by this pull request
@etimberg etimberg changed the title Resolves #10303 Correct auto.esm.js import to allow module to work in browser May 11, 2022
@etimberg
Copy link
Member

I've updated the title

@etimberg etimberg merged commit 4eb65c0 into chartjs:master May 11, 2022
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.

Import in auto.esm.js is wrong
4 participants