Fix package.json entry points

[benmccann]

Closes #9404

[kurkle]

After these changes, A simple, non-module package, requiring chart.js would fail:

require() of ES modules is not supported.

While without these changes both non-module and module packages load chart.js just fine in node.

[kurkle]

It actually looks like current Node.js is unable to work with Dual CommonJS/ES module packages without using the .cjs or .mjs file extension to differentiate the type of one of the exports.

I'm still unsure what the actual issue is with SvelteKit and commonjs modules.

[benmccann]

Duh. I should have caught that. Thanks for reviewing. I've updated it to work with both Node and Vite now. Vite has no issue with CJS modules. If it did then you couldn't run anything with it 😄 It's only server-side UMD that it has an issue with

[kurkle]

It's only server-side UMD that it has an issue with

So it use the esm for client side and the same code is used server side, but it loads the cjs (because of no type="module" or .mjs extension) and thus has different exports and that fails?

[kurkle]

Also, do we really need to have a cjs build, is it just about loading the different package server vs client? I'd assume the UMD package works the same as before (and would not be used in this vite case anymore).

Could also be a bit less breaking to rename the esm build to .mjs and not add the type="module", which would give the same result if I'm not mistaken. Also I'm really not sure about the browser thing, because its for bundlers and those can use the main/module already fine.

[benmccann]

So it use the esm for client side and the same code is used server side, but it loads the cjs (because of no type="module" or .mjs extension) and thus has different exports and that fails?

I believe Vite will prefer browser and module over main on the client and that it tries to use main on the server unless type: "module" is present and then it will use module

do we really need to have a cjs build, is it just about loading the different package server vs client? I'd assume the UMD package works the same as before (and would not be used in this vite case anymore)

Yes, for Vite we would because it needs either CJS or ESM on the server and I believe the only way it will use the ESM is if we tell it type: "module"

Could also be a bit less breaking to rename the esm build to .mjs and not add the type="module", which would give the same result if I'm not mistaken.

Good thought, but I just tested and Vite isn't accepting it if I put .mjs in module and UMD .js in main:

Failed to resolve entry for package "chart.js". The package may have incorrect main/module/exports specified in its package.json

I'm really not sure about the browser thing, because its for bundlers and those can use the main/module already fine

I removed browser. I had added it because I thought it might be a good practice to provide it, but it's not needed as far as I'm aware

[kurkle]

This would be a breaking change, because the format of the main changes from UMD to CJS.

[kurkle]

Just tested, the problem is not with the UMD build, vite works fine if the build is UMD and is named chart.cjs

The proposed changes do not work for the vite bug. Its hard to test, because vite considers linked modules source and just linking chart.js without changes makes it work.

So you'll need to edit it directly in node_modules to see if changes work. I found a three ways to make things work:

• Add type: 'module' and change main to 'dist/chart.esm.js'
• Remove main
• Add some vite config to svelte.config.js:

const config = {
  kit: {
    // hydrate the <div id="svelte"> element in src/app.html
    target: '#svelte',
    vite: {
      ssr: {
        noExternal: ['chart.js']
      }
    },
  },
};

The vite SSR is experimental. I don't think we should "fix" chart.js for something experimental.

[simonbrunel]

Thanks a lot @kurkle for figuring out impacts of this PR but also looking for solutions for Vite/Svelte. I would expect these frameworks to pick the module entry instead of main (as it's done by bundlers), so I'm wondering if it's a bug in Vite/Svelte or not. If so, I fully agree, it should not be fixed in this lib with the risk to break other integrations. If your last solution (i.e. svelte.config.js) works, I think it's an acceptable workaround.

I'm also concerned about the suggested changes because Chart.js is integrated in so many different ways that we can't assume how the UMD build provided as main is consumed. Though, I didn't know that node.js could import ES Modules (at least since v12). I didn't try but if it's possible to require an ES module from a CJS module natively then maybe providing the ESM build as main is a solution. But still, not sure about the real impacts so I would wait a major release.

If we change main, we should also add browser: '<umd>'. IIRC, this field is used by CDNs such as jsdliver, etc. to select which file to serve by default. If not provided, it fallbacks to main. It's also used by webpack.

[etimberg]

I am definitely wary of changing package.json as I think it could easily have unintended effects. Maybe we can add this into the documentation. We already have the "integrations" page, but that is already getting too long. Perhaps it's better to make a section in the left menu for "Advanced Integrations" and add a page for each system that needs custom documentation

[benmccann]

vite considers linked modules source and just linking chart.js without changes makes it work.

Oh wow. That's evil. I'm not sure how you figured that out, but am happy to know that. It does explain some things I was confused by

Thanks @kurkle for the investigation!

I'll go ahead and close this and file an issue to revisit for 4.0