Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SvelteKit build crash #1111

Open
sproott opened this issue Dec 22, 2022 · 16 comments
Open

SvelteKit build crash #1111

sproott opened this issue Dec 22, 2022 · 16 comments

Comments

@sproott
Copy link

sproott commented Dec 22, 2022

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

  • Quirrel version: 1.13.1
  • Node/npm version: Node 16.18.0, npm 8.19.2
@Skn0tt
Copy link
Member

Skn0tt commented Dec 27, 2022

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
Copy link
Author

sproott commented Dec 27, 2022

@Skn0tt would this be a satisfactory solution?

@Skn0tt
Copy link
Member

Skn0tt commented Dec 29, 2022

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
Copy link
Author

sproott commented Dec 29, 2022

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
Copy link
Member

Skn0tt commented Dec 29, 2022

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
Copy link
Author

sproott commented Dec 29, 2022

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
Copy link
Author

sproott commented Dec 29, 2022

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
Copy link
Member

Skn0tt commented Dec 29, 2022

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:

"npm-run-all": "4.1.5",

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
Copy link
Author

sproott commented Dec 29, 2022

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
Copy link
Author

sproott commented Dec 29, 2022

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
Copy link
Author

sproott commented Dec 29, 2022

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
Copy link
Member

Skn0tt commented Dec 30, 2022

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 #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
Copy link
Member

Skn0tt commented Dec 30, 2022

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

@sproott
Copy link
Author

sproott commented Feb 7, 2023

@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
Copy link

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
Copy link

@sproott Try adding this to your vite.config file

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

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

No branches or pull requests

3 participants