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

Libraries not working with svelte-preprocess v6 #643

Closed
flayks opened this issue Jun 23, 2024 · 14 comments
Closed

Libraries not working with svelte-preprocess v6 #643

flayks opened this issue Jun 23, 2024 · 14 comments

Comments

@flayks
Copy link

flayks commented Jun 23, 2024

Describe the bug
After upgrading to svelte-preprocess v6, I get into errors using libraries such as @portabletext/svelte where it just says that PortableText is not defined.

Is it linked to the PortableText lib here or svelte-preprocess? The thing that bugs me is that it works perfectly fine with the v5, without changing any code or other dependencies. Just upgrading to svelte-preprocess v6 breaks it.

Logs

generating static routes 
│ 12:57:47 ▶ src/pages/404.astro
│ 12:57:47   └─ /404.html (+106ms)
│ 12:57:48 ▶ src/pages/index.astro
│ 12:57:48   └─ /index.htmlPortableText is not defined
│   Stack trace:
│     at file:///Users/flayks/sites/portfolio/apps/website/dist/chunks/index_SkFUPnTb.mjs:244:31
│     at file:///Users/flayks/sites/portfolio/apps/website/dist/chunks/index_SkFUPnTb.mjs:338:260
│     at Object.render (file:///Users/flayks/sites/portfolio/apps/website/dist/chunks/Button_C7xTWKNE.mjs:31
│ 2:17)
│     at renderFrameworkComponent (file:///Users/flayks/sites/portfolio/apps/website/dist/chunks/astro/serve
│ r_Bc54YaF1.mjs:1111:66)
│     at async renderComponent (file:///Users/flayks/sites/portfolio/apps/website/dist/chunks/astro/server_B
│ c54YaF1.mjs:1271:10)
│ error: script "build" exited with code 1
│ command finished with error: command (/Users/flayks/sites/portfolio/apps/website) /Users/flayks/.volta/bi
│ n/bun run build exited (1)
└────>
 ERROR  run failed: command  exited (1)
error: script "build" exited with code 1

To Reproduce
A bit trivial to reproduce using this specific example with Sanity and PortableText, but just using 5.1.4 vs 6.0.1 breaks the build.

Expected behavior
It should be normally without error

Information about your project:

  • Arc
  • macOS
  • svelte-preprocess version 6.0.1
  • Astro v4.11
@dummdidumm
Copy link
Member

Please provide a proper reproduction in form of link to a repository on Github or Stackblitz

@flayks
Copy link
Author

flayks commented Jun 24, 2024

@dummdidumm here is a minimal reproduction of my setup using Turborepo, bun and Astro with some Svelte components. There is some SCSS imports for global variables and functions as well as a local variables file.

https://github.com/flayks/svelte-preprocess-v6

If you run bun i && bun run build, you'll have this error:

...

│  building client (vite) 
│ 20:11:27 [types] Added src/env.d.ts type declarations.
│ 20:11:27 [vite] ✓ 1 modules transformed.
│ 20:11:27 [vite] dist/_astro/client.Cx1FBVJX.js  0.61 kB │ gzip: 0.42 kB
│ 20:11:27 [vite] ✓ built in 8ms
│ 
│  generating static routes 
│ 20:11:27 ▶ src/pages/index.astro
│ 20:11:27   └─ /index.htmlPortableText is not defined

...

it is using the v6.0.1 (latest) of svelte-preprocess.

Now if you switch to the branch v5 and run the same exact code, it builds without any issue.

This makes me think that svelte-preprocess v6 is handling things a bit different, although my code might be faulty! But I can't figure out what part if so.

Thank you for digging into this!

@dummdidumm
Copy link
Member

dummdidumm commented Jun 24, 2024

Can you reduce this further and use node instead of Bun (I'm on windows)? I doubt that turborepo or bun have something to do with it.
My hunch is #632 has something to do with this.

@flayks
Copy link
Author

flayks commented Jun 24, 2024

@dummdidumm No worries, I updated both branches to use pnpm instead

Ha! that could potentially be something yes

@dummdidumm
Copy link
Member

I can't reproduce this, I get a different one though: Error while preprocessing C:/repos/svelte/asp/apps/website/src/components/Test/Test.svelte - [sass] Undefined variable.

Please shrink down the reproduction further:

  • no monorepo, just the app with the styles needed in the same folder as the app
  • no turborepo

@flayks
Copy link
Author

flayks commented Jun 26, 2024

Hm, that's weird. I've pushed a branch 'nomonorepo' and it seems to build. Would that mean that it comes from the use of a monorepo?

It's a pity because the whole purpose of this is to share Svelte configs through multiple apps (packages/config/svelte.js),
hence the monorepo (I have 4 apps in totals). Would you know why that happens, if that's really coming from this?

@jacwright
Copy link

I don't have a monorepo and am getting this same error moving from 5.x to 6. It is occurring with all my imports in the Svelte files, as if the imports don't exist. The error only happens at runtime for me, not compile time. But I can see the errors in VSCode as warnings.

Screenshot 2024-06-27 at 11 07 17 AM Screenshot 2024-06-27 at 11 09 36 AM

I'll see if I can get a reproduction put together.

@jacwright
Copy link

jacwright commented Jun 27, 2024

I found it is occurring when verbatimModuleSyntax is not defined or is false. This is the issue. But our app is very large, so there is a lot of busy work to go through every file (both .Svelte and .ts) to add the type keyword to support this requirement. How necessary is verbatimModuleSyntax, exactly? Since 5.x was able to get by without it. 😬

If you'd like me to provide a simple example I can, but you can probably turn off verbatimModuleSyntax and reproduce in any project.

Edit: we'll probably stick with 5.x for now so we don't have to refactor our whole app. It doesn't seem we're missing out on any features or benefits with 6.x.

@flayks
Copy link
Author

flayks commented Jul 1, 2024

After trying this on another whole site and setup, I can assure that the issue comes from svelte-preprocess v6. Same errors, internal svelte tools would not be found, types missing, etc. Sticking to v5 for now as well but I think there is a real issue with it @dummdidumm.

@dummdidumm
Copy link
Member

@jacwright this is noted in the list of breaking changes. It is required because there were previously two other options, both of which are deprecated/removed:

  • it was possible to get the same using some combination of certain tsconfig options (preserveValueImports etc) which are all deprecated by TypeScript and cease to function in TS5.5+
  • there was a black-magic/best-effort transpiler within the preprocessor that made it work which is removed in version 6

My guess is you were probably implicitly using the latter. As for the large app and the refactoring: It's "only" a matter of adding type prefixes to the imports, e.g. import { someValueImport, SomeTypeImport } from 'somewhere' becomes import { someValueImport, type SomeTypeImport } from 'somewhere'. So while it's a lot of busywork it's nothing that can really go wrong or changes the behavior of your code.

@flayks from what I can see your issue looks different though, so these are unrelated things.

@jacwright
Copy link

@dummdidumm yeah, I know. We can do it, but it is hundreds of files. And we have to do it over everything, not just Svelte files. [sigh]

@dummdidumm
Copy link
Member

@jacwright yeah it's tedious for sure - wondering if there's a way to write a migration script for this.

@flayks I finally got to the bottom of this. The sass error I mentioned was a red herring - it came from an unrelated Astro bug where configuration files are resolved from on Windows. When I fixed that locally I finally saw the error.

Turns out it's in fact the same verbatimModuleSyntax problem. When the preprocessor reads the files, it tries to find the closest tsconfig for each. For those inside the components it doesn't find one (because there is none), so it falls back to nothing, which also means verbatimModuleSyntax is unset.

You can fix this by either adding a tsconfig.json with right config to the components package or by setting the typescript.tsconfigDirectory option for sveltePreprocess.

I'll also add additional logic to set verbatimModuleSyntax to true in case no config is found.

dummdidumm added a commit that referenced this issue Jul 9, 2024
In case no tsconfig is found whatsoever, enable `verbatimModuleSyntax` to prevent stripping unused imports.

Also fix a related issue where the config passed manually was not properly parsed.

Also print warning message in bold to make it more visible.

related to #643
dummdidumm added a commit that referenced this issue Jul 9, 2024
In case no tsconfig is found whatsoever, enable `verbatimModuleSyntax` to prevent stripping unused imports.

Also fix a related issue where the config passed manually was not properly parsed.

Also print warning message in bold to make it more visible.

related to #643
@flayks
Copy link
Author

flayks commented Jul 9, 2024

Ha, great job @dummdidumm! Thanks for digging into this and finding the culprit. I updated to 6.0.2 and can actually build without any issue. I'll still do these changes though.

@benmccann
Copy link
Member

@jacwright could you run eslint --fix with this rule to do the migration? The docs say not to use it with verbatimModuleSyntax as there are some differences, but it sounds mostly aligned and looks perhaps like if you ran it as a onetime fix and then removed that it would get you 95% of the way there.

https://typescript-eslint.io/rules/consistent-type-imports/

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

4 participants