quirrel-dev / quirrel

The Task Queueing Solution for Serverless.
https://quirrel.dev
MIT License
885 stars 67 forks source link

SvelteKit build crash #1111

Open sproott opened 1 year ago

sproott commented 1 year ago

Bug Report

Current Behavior

When making a production build of a SvelteKit app that uses Quirrel, it crashes with the following error:

image

Input Code

This is an empty SvelteKit app with one job queue added, which is enough to make the build crash: https://github.com/sproott/quirrel-sveltekit-build-bug

Expected behavior/code

The app should build successfully.

Environment

Skn0tt commented 1 year ago

Hi @sproott, thanks for providing the reproduction repo! Much appreciated :)

Interestingly, when I change the import in routes/job/+server.ts from quirrel/sveltekit to quirrel/sveltekit.cjs, it works just fine.

This seems like a ESM-related bug. If you look at https://www.npmjs.com/package/quirrel?activeTab=explore, you can see how this works: quirrel/sveltekit.mjs links to dist/esm/src/sveltekit.js, and that contains export function Queue. From what i'm seeing, the Quirrel side of things works fine. Could you check with SvelteKit if there's similar errors for other projects? I could imagine this to be a bug in the SvelteKit compiler.

sproott commented 1 year ago

@Skn0tt would this be a satisfactory solution?

Skn0tt commented 1 year ago

I tried out adding "type": "module" to the nearest package.json, but that didn't solve the problem for me 🤔

The whole ESM / CJS thing I don't have a lot of knowledge about. Did you find a workaround for this? I'm really at a loss ...

sproott commented 1 year ago

I'll try to come up with something. How do you test libraries locally? Do you somehow get a project to use a local version? Or do you modify it in node_modules directly somehow?

Skn0tt commented 1 year ago

I tested it by modifying node_modules. For more complex changes, you can clone the Quirrel repo, run npm run build, and then use npm link to test locally.

sproott commented 1 year ago

Where do I get run-s? It's used to run all the build scripts. I tried installing the run-s package, but it still says the executable doesn't exist.

sproott commented 1 year ago

I could take a look at simplifying the whole build process to avoid using the generate-proxies.sh script. Perhaps using tsup as it works nicely for building TS libraries into multiple module formats.

Skn0tt commented 1 year ago

Where do I get run-s?

run-s is part of npm-run-all, which should be installed as part of Quirrel's dev dependencies:

https://github.com/quirrel-dev/quirrel/blob/58bb9427a9426c2048528deeefd83ac3ff9ed98c/package.json#L61

I could take a look at simplifying the whole build process to avoid using the generate-proxies.sh script. Perhaps using tsup as it works nicely for building TS libraries into multiple module formats.

I'd prefer to keep the changes minimal, so we don't risk breaking any other setups.

sproott commented 1 year ago

I have already whipped up a version using tsup which works properly without the need to generate proxies and polluting the main library dir. I'll put it on my forked repo so you can take a look, I think it would be a step in the right direction to modernize the build process a bit.

EDIT: here is the tsup version.

But I'll continue to investigate the type: module fix. I think it will be enough to change the generate-proxies script, because with type: module the structure of reexports should be a bit different.

sproott commented 1 year ago

Here's the simpler fix using type: module and making the .js file export the ESM version. Not sure whether this change will break any other use cases, but afaik it shouldn't.

sproott commented 1 year ago

Also, I think the generate-proxies.sh script could be avoided even with the current build process by using the exports and typesVersions fields in package.json to export the files under dist/ as top level, as seen in the tsup version.

Skn0tt commented 1 year ago

Thanks so much for opening the two PRs! I looked at the TSUP one, and it seems like that results in bundled output (and can't be turned off). Instead of bundling all files into one, I'd like to keep the dist directory in the same shape as src, as this makes both debugging & working with static files like the UI much simpler.

So I looked into https://github.com/quirrel-dev/quirrel/pull/1116, which looks good generally. I've found a couple of spots where it breaks existing imports, and at this point, i'm seriously debating if I should just switch this to ESM-only. ESM has been supported since Node v10, and Quirrel only supports Node v14 onwards. I'll work on a PR to switch over to ESM-only, let's see how good that goes.

Skn0tt commented 1 year ago

Here's my attempt at migrating to ESM, but it's a really big lift and I hope it's worth the hassle 😅 https://github.com/quirrel-dev/quirrel/pull/1117 There's a couple of things missing, for example JSON imports. I'll have another look at it next week.

sproott commented 1 year ago

@Skn0tt I don't know if you've done any more progress, but I've found this: https://github.com/unjs/unbuild which supports output without bundling and has a nice frontend for configuration.

ghostwalkerj commented 1 year ago

I have this problem also. Although adding the .cjs allows my project to compile, I lose type definitions. Is there any way we can fix the problem? I'm loving quirrel in my sveltekit project. It's exactly the missing functionality I need.

ghostwalkerj commented 1 year ago

@sproott Try adding this to your vite.config file

ssr: { noExternal: ['quirrel/**'], }