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

Svelte 5 ecosystem CI is failing #12686

Closed
benmccann opened this issue Sep 18, 2024 · 6 comments · Fixed by #12822
Closed

Svelte 5 ecosystem CI is failing #12686

benmccann opened this issue Sep 18, 2024 · 6 comments · Fixed by #12822
Labels

Comments

@benmccann
Copy link
Member

benmccann commented Sep 18, 2024

Describe the bug

Screenshot from 2024-09-18 10-43-48

I don't understand why this is failing though

Screenshot from 2024-09-18 10-43-18

I should also note that TemplateNode is declared in the types twice. Once in the root of the types as a non-exported type and once exported from 'svelte/types/compiler/interfaces'

Reproduction

Upgrade enhanced-img to the latest Svelte 5 and run pnpm check

Logs

No response

System Info

`main`

Severity

blocking an upgrade

Additional Information

Should have been fixed with sveltejs/svelte#12292, but for some reason it seems not to be working

@dummdidumm
Copy link
Member

There's no real good to make this work other than to keep exposing the old AST types through that never-meant-to-be-public interface. And I'm not sure if that is the right solution. I'd rather switch to redeclaring those types within enhanced image for the time being, and once it has a peer dependency of Svelte 5 minimum you can switch to a) the modern parse output b) use the types exported along with it.

@dummdidumm dummdidumm removed their assignment Oct 12, 2024
@benmccann
Copy link
Member Author

Ah, we can switch to modern parse output. I didn't realize until just the other day that there's a flag for that and it can return two different AST.

It's probably a bit late in the game, but I would have made it version: 5 so that we don't have moreModern: true if we switch to some other implementation in the future.

@dummdidumm
Copy link
Member

The old AST will probably be gone in 6, and I sure don't hope we already have changes by then, so if we ever have another AST we can just use modern again

@benmccann
Copy link
Member Author

Oh yeah, good call. Maybe we should deprecate the non-modern one now.

@dummdidumm
Copy link
Member

I mean, it is deprecated. It's just for a seamless switch that we default to the old one

@benmccann
Copy link
Member Author

It's not showing deprecated for me. I upgraded the project to Svelte 5, but don't see any indication in the IDE.

Screenshot from 2024-10-15 13-13-02

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants