fix(build): svelte packaging

[benmccann]

Updates

Hi, I'm a SvelteKit maintainer and got pointed to various issues with SvelteKit compatibility such as metonym/carbon-charts-svelte-examples#1 and #1148

Svelte packages should only distribute ESM. main and svelte should point to the same thing

Also, UMD should only be used if you want to use it in a browser <script> tag. CJS generally has far better compatibility with bundlers and with Node.js

Demo screenshot or recording

N/A

Review checklist (for reviewers only)

• Demos all features
• Documented/annotated
• Matches UI/UX specs
• Meets the code style guide
• Accessible
• Mobile first (responsive)
• RTL support (bidirectional text)
• Performant (limited bloat)

[metonym]

@benmccann As always, thank you for taking the time. This is a great improvement.

I've built and tested this locally. I no longer encounter the error as described in: metonym/carbon-charts-svelte-examples#1

However, one small runtime error I faced was that the svelte/main entry should still be src/index.js as the folder is copied over to dist when building for production.

The published assets have the following structure:

I still see the vite SSR errors in the console, but it is non-blocking as the app successfully runs in development:

[metonym]

For the svelte package, if both "svelte" and "main" entries point to the uncompiled Svelte source files, we can do away with Rollup entirely.

[benmccann]

It would work to point to either a bundled or unbundled entry point as long as it's ESM. However, there are performance benefits to pointing to a bundled file (sveltejs/kit#2612) and you may get bug reports about slowness if you point at the unbundled version. That's why I pointed to the bundled version instead, so I'd probably lean against changing it to the source version

[benmccann]

Actually, I take it back. It's much better to just distribute the source. Svelte expects uncompiled .svelte files to be distributed that way all .svelte files can be built with the same version of the Svelte compiler (vs if some were compiled when they were packaged it might be with a different version of the compiler). I've gone ahead and removed the Rollup config so that the Svelte plugin won't get executed and just the source is distributed

[metonym]

Awesome. One last requested change: build.sh needs to be adjusted as it will throw an error because it cannot detect rollup.config.js.

# svelte/build.sh

rm -rf dist
+ mkdir dist

- echo "bundling..."
- rollup -c

[benmccann]

good catch. done!

[theiliad]

@benmccann Seems like we're introducing a breaking change by switching the core bundle from umd to cjs.

Why not also offer a umd bundle?

[benmccann]

Thanks for taking a look @theiliad! UMD modules try to guess at runtime which module system they're being used in, and then act as that kind of module. The idea is that you can load the file in a plain <script> or Node.js or whatever and have it work. At least that's the idea. In practice, they cause all sorts of trouble if you're trying to use them with bundlers. So I guess the question is how is this script expected to be used? Because if it's only expected to be used by bundlers and Node.js then it's not breaking and will fix a number of issues. If it's expected to be used directly in a <script> tag then if would be valuable to distribute a UMD version still. That wasn't something I saw in the docs though, so is it a supported use case? And what would it mean to distribute it, but not reference it from the package.json or how would you like it to be referenced there? Because main should point to a CJS version for the best Node.js and bundler compatibility

[theiliad]

Thanks for taking a look @theiliad! UMD modules try to guess at runtime which module system they're being used in, and then act as that kind of module. The idea is that you can load the file in a plain <script> or Node.js or whatever and have it work. At least that's the idea. In practice, they cause all sorts of trouble if you're trying to use them with bundlers. So I guess the question is how is this script expected to be used? Because if it's only expected to be used by bundlers and Node.js then it's not breaking and will fix a number of issues. If it's expected to be used directly in a <script> tag then if would be valuable to distribute a UMD version still. That wasn't something I saw in the docs though, so is it a supported use case? And what would it mean to distribute it, but not reference it from the package.json or how would you like it to be referenced there? Because main should point to a CJS version for the best Node.js and bundler compatibility

No worries!

Yes we do have umd users

AFAIK main usually is umd, as bundlers usually look at the module field... @metonym feel free to chime in if you'd like

Also found this just searching https://2ality.com/2017/04/setting-up-multi-platform-packages.html

[benmccann]

bundlers usually look at the module field

Yes, that's mostly true. Most bundlers look at things in the following order: exports, browser, module, main. Though it depends on the bundler and project configuration

AFAIK main usually is umd

It's quite rare in my experience. What I have seen somewhat frequently though is to list UMD in fields used by CDNs like "jsdelivr" or "unpkg". Since UMD packages are primarily used in <script> tags that lets you list Node-compatible packages in main and deliver the UMD package via CDN.

Putting a UMD package in main often fails with Vite because Vite tries to load the main directly in Node.js without any bundling

Yes we do have umd users

Do you know anything else about how they use it? E.g. do they have a build process that includes it via <script> tag or use it from a specific CDN or something? We can certainly design this to be non-breaking for certain use cases, but would need to know what they are.

Also found this just searching https://2ality.com/2017/04/setting-up-multi-platform-packages.html

It's a starting point to understanding this stuff, but I wouldn't rely on it too heavily since it's from 2017. E.g. it doesn't talk about exports which are really important today. Webpack 5 has been released which does some of this slightly differently. And there's a new generation of bundlers like Vite, esbuild, and swc that the article doesn't mention at all.

[benmccann]

Any thoughts about this one?

[metonym]

@theiliad If users do use the UMD version, would the following be amenable?

• generate UMD and keep the bundle.js name, but set the entry point as "browser": "./bundle.js"; this way, the import path in a script would still work
• ship CJS as the "main" format but use a different name than "bundle.js"

[theiliad]

@theiliad If users do use the UMD version, would the following be amenable?

• generate UMD and keep the bundle.js name, but set the entry point as "browser": "./bundle.js"; this way, the import path in a script would still work
• ship CJS as the "main" format but use a different name than "bundle.js"

Yes I think that works. Although I see it a lot, I'm also not a big fan of having UMD be the main.

The reason for that is even say in a react project with some bundlers you could end up (unintentionally) using UMD in dependencies... which I think destroys tree-shaking...

So say you're using @carbon/charts-react, when that pulls in @carbon/charts it could pull in the UMD version in some setups, depending on what bundler and configs you're using.

[theiliad]

@benmccann are you able to apply these changes?

Also, where do you see the names of fields that jsdelivr or other CDNs use?
I think if we can also add a few CDN-specific fields that could be a great piece...

[benmccann]

I don't think that we'd want to add a UMD file in the browser field because that would break most projects. Most bundlers will use the browser field before module. Right now they're finding the module field and using the ESM file provided there, but if you make them find a UMD file first they will break.

I'm happy to add the UMD file with fields for the CDNs. Here are the main ones I've seen used in other projects:

    "jsdelivr": "dist/bundle.umd.js",
    "unpkg": "dist/bundle.umd.js",

[theiliad]

I don't think that we'd want to add a UMD file in the browser field because that would break most projects. Most bundlers will use the browser field before module. Right now they're finding the module field and using the ESM file provided there, but if you make them find a UMD file first they will break.

I'm happy to add the UMD file with fields for the CDNs. Here are the main ones I've seen used in other projects:

    "jsdelivr": "dist/bundle.umd.js",
    "unpkg": "dist/bundle.umd.js",

Let's read some more on bundling & package.json as I think the way that they're done has changed in the past 1-2 yrs. Lmk if you encounter anything interesting

[benmccann]

Let's read some more on bundling & package.json as I think the way that they're done has changed in the past 1-2 yrs. Lmk if you encounter anything interesting

I'm pretty up-to-date and familiar with the bundling stuff. Are there any specific questions I can help answer?

Here are some helpful references:

• https://webpack.js.org/configuration/resolve/#resolvemainfields
• https://vitejs.dev/config/#resolve-mainfields
• https://github.com/rollup/plugins/tree/master/packages/node-resolve/#package-entrypoints
• https://nodejs.org/api/packages.html#package-entry-points

[theiliad]

Let's read some more on bundling & package.json as I think the way that they're done has changed in the past 1-2 yrs. Lmk if you encounter anything interesting

I'm pretty up-to-date and familiar with the bundling stuff. Are there any specific questions I can help answer?

Here are some helpful references:

• https://webpack.js.org/configuration/resolve/#resolvemainfields
• https://vitejs.dev/config/#resolve-mainfields
• https://github.com/rollup/plugins/tree/master/packages/node-resolve/#package-entrypoints
• https://nodejs.org/api/packages.html#package-entry-points

Thanks Ben.

So seems like the default values on most of these use main as a last resort. So with that in mind, why not point main to UMD?

then Module and browser could be our es6-like entrypoints

[benmccann]

The issue is with NodeJS. The only thing it prefers above main is exports and the package doesn't define an exports. Most users won't use NodeJS directly, but only after bundling and won't encounter this. However, Vite does unbundled development and calls Node directly and that causes issues. Defining exports could be another solution, so that you could leave main as is and nothing would use it. But that seems like a riskier change because adding exports prevents anyone from doing deep imports and if nothing is going to be using the value of main then why not just change it?

[theiliad]

The issue is with NodeJS. The only thing it prefers above main is exports and the package doesn't define an exports. Most users won't use NodeJS directly, but only after bundling and won't encounter this. However, Vite does unbundled development and calls Node directly and that causes issues. Defining exports could be another solution, so that you could leave main as is and nothing would use it. But that seems like a riskier change because adding exports prevents anyone from doing deep imports and if nothing is going to be using the value of main then why not just change it?

Why would someone use this in NodeJS?

[benmccann]

SvelteKit is isomorphic. It will render all code on the server and the client by default the way the instructions are setup: https://github.com/carbon-design-system/carbon-charts/tree/master/packages/svelte#basic

That should not be an issue with most libraries. But if you want to say that it's only supported in the browser then the docs should be updated to recommend only doing something like https://github.com/carbon-design-system/carbon-charts/tree/master/packages/svelte#dynamic-import

[theiliad]

SvelteKit is isomorphic. It will render all code on the server and the client by default the way the instructions are setup: https://github.com/carbon-design-system/carbon-charts/tree/master/packages/svelte#basic

That should not be an issue with most libraries. But if you want to say that it's only supported in the browser then the docs should be updated to recommend only doing something like https://github.com/carbon-design-system/carbon-charts/tree/master/packages/svelte#dynamic-import

Does svelteKit not offer a way to choose the main field used?

[benmccann]

SvelteKit uses Vite and Vite does provide a way to choose the main field, but in practice that's a pretty useless feature (not just in Vite, but in most bundlers) because it applies to all packages and if you change it then you'll almost certainly break some other package.

But perhaps more importantly, Vite isn't heavily involved here and that option wouldn't affect this code path. Vite is not bundling this code and is just asking NodeJS to run it. It's the node package resolution algorithms that get invoked here that cause the main field to get picked up and for the UMD file to get loaded into Node and cause issues. You'd have to ask Node to do something else and I don't know that Node's resolution algorithms are configurable and even if they were it would be a large change to Vite to introduce a new option to configure that on a per-package and it's a worse user experience for the users of this package to ask them to configure their package resolution options vs. simply making it work out-of-the-box.

[benmccann]

I've reverted the change to the core package for now. I discovered another issue with the way it's packaged and I think it's better to address separately. And in the meantime I'd love to at least get the change to the Svelte package checked in since it's less controversial 😄

[theiliad]

I've reverted the change to the core package for now. I discovered another issue with the way it's packaged and I think it's better to address separately. And in the meantime I'd love to at least get the change to the Svelte package checked in since it's less controversial 😄

@metonym what are you thinking here?

As mentioned earlier I'm not a big fan of UMD being the main entrypoint, but I thought with module & browser we could have that happen.

I don't particularly mind switching out our main, however let's keep bundle.js a UMD bundle just so we're not breaking CDN usages...

[benmccann]

I've been doing some more testing and am actually not sure that it'd fix it. Or at least that change alone might not fix it. I'd feel better about merging this with just the Svelte changes actually and coming back to the core changes after I've had more time to test.

It's really hard for me to test this because I can't get the library to build. node-gyp bombs out trying to build node-sass. Is there some way you could upgrade (#1172) or remove node-sass? It appears to be quite an old version and perhaps optional. I'm not that familiar with yarn and that's making it hard for me. (I'd recommend pnpm if you have any interest in alternatives. SvelteKit and Vite have both switched to it). I don't really know that it's used in the core package because I see it referred to at least in one place in the yarn.lock as optional and I installed with npm and it didn't install node-sass, so I'm wondering if there's some way we can get yarn to stop trying to install it when installing dependencies for the core package?

I ran into another issue while trying to use npm that perhaps you could fix as well? #1277 I'm unable to send a PR with the fix for that one since I can't get yarn to work to update the yarn.lock

[theiliad]

I've been doing some more testing and am actually not sure that it'd fix it. Or at least that change alone might not fix it. I'd feel better about merging this with just the Svelte changes actually and coming back to the core changes after I've had more time to test.

It's really hard for me to test this because I can't get the library to build. node-gyp bombs out trying to build node-sass. Is there some way you could upgrade (#1172) or remove node-sass? It appears to be quite an old version and perhaps optional. I'm not that familiar with yarn and that's making it hard for me. (I'd recommend pnpm if you have any interest in alternatives. SvelteKit and Vite have both switched to it). I don't really know that it's used in the core package because I see it referred to at least in one place in the yarn.lock as optional and I installed with npm and it didn't install node-sass, so I'm wondering if there's some way we can get yarn to stop trying to install it when installing dependencies for the core package?

I ran into another issue while trying to use npm that perhaps you could fix as well? #1277 I'm unable to send a PR with the fix for that one since I can't get yarn to work to update the yarn.lock

Last time I looked into this, the angular package I think was the only reason why node-sass was coming in. We'd need to update some of those deps and get a more modern setup going for that package.

[benmccann]

Is there some way I can build the core package that doesn't involve installing the dependencies for the angular package?

[metonym]

I pulled the latest changes from this PR and built charts locally. I then manually copied the dist folders for @carbon/charts and @carbon/charts-svelte to the node_modules folder.

I still see this error, but after reloading the page it works as expected:

After a page reload:

[benmccann]

Yes, that's the same as now. This cleans things up a bit, but doesn't solve all issues. I'd like to do the rest in a follow-up if that's okay since the discussion on this PR has already gotten quite long.

[metonym]

@benmccann Fine by me 👍

The update to the Svelte chart wrappers is an improvement though; it removes this vite warning:

Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.

[benmccann]

I finally got this package building. I had to downgrade to an older version of Node. So I was now able to track down the problem and left a comment on the issue explaining the problem: metonym/carbon-charts-svelte-examples#1 (comment)

[theiliad]

mostly going off of @metonym 's approval here since it's svelte

[theiliad]

Thanks @benmccann!