Skip to content

Fix: Add missing registerables key to UMD exports#10584

Closed
GuyGooL5 wants to merge 2 commits into
chartjs:masterfrom
GuyGooL5:fix/registerables-umd
Closed

Fix: Add missing registerables key to UMD exports#10584
GuyGooL5 wants to merge 2 commits into
chartjs:masterfrom
GuyGooL5:fix/registerables-umd

Conversation

@GuyGooL5

@GuyGooL5 GuyGooL5 commented Aug 9, 2022

Copy link
Copy Markdown

During Next.js development, I and @zivnadel found that the registerables export was missing from the UMD exports while trying to do Server-Side-Rendering.
The problem didn't re-appear with using Client-Side-Rendering.

After a thorough investigation, we found that it was not exported from src/index.umd.js so we added it.

@LeeLenaleee LeeLenaleee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The registerables are not used in the code so afaik it should not break on that.

Also it seems there are 2 extra indents that should not be there

Comment thread src/index.umd.js Outdated
@benmccann

Copy link
Copy Markdown
Contributor

I would highly recommend not using with UMD version with Next.js. You should use the Node version instead (ESM)

Co-authored-by: Jacco van den Berg <jaccoberg2281@gmail.com>
@GuyGooL5

Copy link
Copy Markdown
Author

@LeeLenaleee hi, sorry for the indentation, probably the formatting did that. About the usage of registerables, it is documented here.
@benmccann I'm not explicitly importing the UMD version. What I do is

import { registerables } from "chart.js";

and probably Next.js' webpack selects the base file dist/chart.js by itself when doing SSR, in which registerables is not exported nor exists.

@GuyGooL5

Copy link
Copy Markdown
Author

@LeeLenaleee as you can see, registerables is indeed exported in the esm version

Chart.js/src/index.js

Lines 20 to 25 in 1551537

export const registerables = [
controllers,
elements,
plugins,
scales,
];

@benmccann

Copy link
Copy Markdown
Contributor

Yeah, Chart.js 3.x has non-ideal behavior in its package.json. In Chart.js 4.x you will get ESM instead of UMD so it should work as expected. For now, I would recommend just forcing ESM rather than us making any changes here. E.g. you should be able to do something along the lines of import { registerables } from "chart.js/dist/chart.mjs";

@GuyGooL5

Copy link
Copy Markdown
Author

@benmccann I don't get why should one use a "hackish" way to do stuff.
This feature has already existed and is documented, it was probably just forgotten.

@LeeLenaleee

Copy link
Copy Markdown
Collaborator

It is not forgotten, the UMD build does not export anything except for the chart itself, only the ESM build exports the registerables that's why its under that sub category since bundlers use the ESM build.

The UMD build also has no use case for the registerables since it already registers everything.

You can try to use this import instead: import Chart from 'chart.js/auto'; otherwise it should be resolved with V4. Since what you are proposing is a kind of a hacky solution too its not the intended way of using it.

@GuyGooL5

Copy link
Copy Markdown
Author

@LeeLenaleee I understand your point now, but how would one intuitively understand that, because it was a terrible debugging session to find out that registerables is not exported when doing SSR because it bundlers with CommonJS or something, yet when doing CSR it works just fine. Maybe giving a hint update in the documentation 🤔

@LeeLenaleee

Copy link
Copy Markdown
Collaborator

Because we did not know that SSR in next.js handled it this way, because all other bundler imports always work fine. And as @benmccann pointed out this will likely be resolved in V4 (where we are working on now) so then it wont be an issue anymore.

@kurkle

kurkle commented Aug 18, 2022

Copy link
Copy Markdown
Member

I believe this was fixed by #10525

@benmccann

Copy link
Copy Markdown
Contributor

Yes, it was. I don't think we should merge this

@GuyGooL5

Copy link
Copy Markdown
Author

@kurkle @benmccann hi, I've seen the changes you referred me to, and registerables is indeed exported from the ESM version, but not from the UMD version. As you can see here, it doesn't yet exist.

@benmccann

Copy link
Copy Markdown
Contributor

Yes, which is correct. The change that will help you is that the main in package.json no longer points to the UMD version, so Node will not use it.

@LeeLenaleee

Copy link
Copy Markdown
Collaborator

What do we want to do with this since this should be resolved by #10525 and the UMD build should not need the registerables so I think we can close this. Because this feels like a hacky fix for a better solution we already have

Any objections to closing this?

@kurkle

kurkle commented Sep 15, 2022

Copy link
Copy Markdown
Member

I'll just close this. We can always re-open if needed.

@kurkle kurkle closed this Sep 15, 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.

5 participants