Skip to content

fix: allow require statement in exports.#10914

Closed
lucien-perouze wants to merge 1 commit into
chartjs:masterfrom
lucien-perouze:master
Closed

fix: allow require statement in exports.#10914
lucien-perouze wants to merge 1 commit into
chartjs:masterfrom
lucien-perouze:master

Conversation

@lucien-perouze

@lucien-perouze lucien-perouze commented Nov 24, 2022

Copy link
Copy Markdown

Hello first contribution on open source project and don't have a clue what's the protocol nor time to read all your docs sorry :/

But here is a really obvious quick fix for a big issue : We cannot require() your library.

As mentioned in this issue that I commented :
#10911

When specifying exports in package.json it overwrites the main attributes.
So currently there is no rule for "requiring" the module.
I simply added a default rule to package.json instead of the import one and it works like a charm.

Thank you for your work, best regards,

Lucien Perouze

@dangreen

Copy link
Copy Markdown
Collaborator

@lucien-perouze Hi.

We cannot require() your library

Chart.js v4 is an ESM-only package, so you can't require it

@lucien-perouze

Copy link
Copy Markdown
Author

Why so ? Is there features that would break ?
I'm developing a component library with Chart.js and I need to build it in common js for compatibility purposes.
It works well with my fix but I haven't tested all your features.

@dangreen

Copy link
Copy Markdown
Collaborator

@lucien-perouze Today many packages have become ESM-only. It is important to push forward ESM adoption.

@LeeLenaleee

Copy link
Copy Markdown
Collaborator

As stated in the docs you need to require the UMD build specifically, that should work fine

https://www.chartjs.org/docs/master/getting-started/integration.html#requirejs

@kurkle

kurkle commented Dec 11, 2022

Copy link
Copy Markdown
Member

@lucien-perouze Today many packages have become ESM-only. It is important to push forward ESM adoption.

I don't really understand. I think its important to be ESM-first, but why is it important to not support commonjs?

I think it would require something like that: https://github.com/chartjs/Chart.js/compare/master...kurkle:Chart.js:node-cjs?expand=1

@simonbrunel

Copy link
Copy Markdown
Member

@dangreen Is CommonJS dead / deprecated? I agree with @kurkle, that's a strange decision to actually drop support for it. From who this decision comes from and why? Maybe I'm missing some important points so any links to issues / PR would be appreciated.

Chart.js is used for server-side rendering and is integrated in CJS projects so it's indeed a bad decision IMO to make such cases more complex or impossible. Is it also because of that change that we need to "fix" all plugins? Before going further adapting plugins I maintain, I would love to have more understanding about this breaking change.

@dangreen

Copy link
Copy Markdown
Collaborator

@kurkle

kurkle commented Dec 15, 2022

Copy link
Copy Markdown
Member

I'd be open to revert the seemingly bad decision.

@dangreen

Copy link
Copy Markdown
Collaborator

@kurkle #10984

@kurkle

kurkle commented Dec 15, 2022

Copy link
Copy Markdown
Member

#10984

@kurkle kurkle closed this Dec 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.

6 participants