Handle CommonJS module

[liamdiprose]

Fixes #868

The vega-embed package is not of "type": "module", so it has to be imported this way.

It seems to work for me in a stripped-down version of this package using Vite 5.

[domoritz]

Can you bump the vite dependency in this pull request and make sure that all the examples and storybook still work?

[liamdiprose]

I'll give it a go on Monday. Enjoy your weekend!

[liamdiprose]

Hey, I checked the examples/storybook as you asked and found they didn't work. I needed to change it to this import * as pkg import pattern in order to get it working for both my project and this project's examples.

[liamdiprose]

I haven't added the yarn.lock file. Do you want me to include it?

[domoritz]

Yes. We should make the ci fail if the lock file is not up to date.

[liamdiprose]

Apparently yarn --frozen-lockfile is supposed to error if it isn't up to date, and yet "all checks have passed".

[domoritz]

Yeah, I expected that to error out. Weird.

[domoritz]

Hmm, does storybook work for you? I am seeing this error on this branch.

[liamdiprose]

My bad - thanks for catching this. Turns out import * from ... pulls in a bunch of stuff meant for node only. I've revisited the import line again, and now we have something similar to before, but the vega-embed.module.js file is resolved manually.

Storybook, Samples, and my project work now (for me).

[domoritz]

Hmm, why doesn't vite pull in the module file automatically? We already specify module in the package.json. Maybe we need to add an explicit export map?

[domoritz]

Hmm, looks like vite prefers browser over module: https://vitejs.dev/config/shared-options#resolve-mainfields. Not sure why, though.

[domoritz]

Adding explicit exports for embed seems to work (vega/vega-embed#1301) but now vite doesn't like fast-json-patch which again to me doesn't make sense since the package is esm as well.

[vite] Named export 'applyPatch' not found. The requested module 'fast-json-patch' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'fast-json-patch';
const {applyPatch} = pkg;

[domoritz]

That doesn't feel right as it requires a specific file in a specific place.

The right fix seems to add import maps.

[domoritz]

This branch isn't working for me btw. This is the svelte kit example.

[liamdiprose]

Yeah, I'm getting the same error as you for sveltekit. Svelte works, which suggests that vite is resolving it correctly, but node isn't.

Storybook was still broken, but I got it to work by setting this resolve.alias: vitejs/vite#9330 (comment) . This is hacky, because its caused by a browser trying to import a nodejs-only module (node-fetch). It would be better to stop it happening in the first place.

I wonder if node expects require() for CJS dependencies (vega-embed). Maybe there is a vite plugin and/or polyfill for this?

I had a go at getting vite to resolve the path to vega-embed.module.js using resolve.alias, but had no luck. I understand why you don't want to assume the file structure of a dependency, but without changing it to "type": "module", I'm pessimistic that node will find it properly. If we must resolve the module's path manually here, then wouldn't it be best to leave it in the most obvious place so it can be debugged easier? (opposed to hiding it as a build-system rewrite plugin)

In summary, its getting messy. I'm going to revisit this in a day.

[domoritz]

I'm happy to change embed to use exports to convince Vite that Vega embed is an esm dependency.

[domoritz]

Marking as draft since it doesn't work yet.

[liamdiprose]

Sorry the stalled progress. I've got a carbon-copy of the svelte-vega package working with the initial commit of this PR.

While I can appreciate the structure of this repo, I can't justify working on Storybook when we only need a svelte-friendly wrapper for vegalite.

[domoritz]

Makes sense. I think storybook isn't the issue big here but svelte kit doesn't work either.

I'll close the pull request for now but it's still something we should address since many people use vite 5.