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

feat(svelte5): add support for Svelte 5 modules #283

Merged
merged 20 commits into from
Jun 10, 2024

Conversation

mcous
Copy link
Contributor

@mcous mcous commented May 15, 2024

Overview

This is a little proof of concept solution for #282. I'm a little unsure of how to add tests, given the existing testing setup. I could use guidance if anyone could offer some!

I'm currently running smoke tests locally against testing-library/svelte-testing-library#375

Change log

  • Add call to compileModule if Svelte 5 is detected and the filename matches the module pattern (.svelte.js)

@sebastianrothe
Copy link
Collaborator

Thanks, I can have a look at it later on Thursday.

@sebastianrothe sebastianrothe self-assigned this May 17, 2024
@sebastianrothe
Copy link
Collaborator

Looks good, I just want to make little adjustments with the imports. So it matches the current style. I will finish this until Monday. Sorry, busy week and weekend :)

@mcous
Copy link
Contributor Author

mcous commented May 18, 2024

No worries, I appreciate your time, and please adjust as you see fit! Note that I went with wildcard imports for the svelte module for technical reasons: compileModule is not an available import in Svelte earlier than v5.

I also thought it might be useful to add a really minimal Svelte 5 e2e test, let me know if that would be helpful for a follow up PR

@mcous mcous marked this pull request as ready for review May 18, 2024 17:45
@mcous
Copy link
Contributor Author

mcous commented May 20, 2024

@sebastianrothe I'm seeing some... interesting results while writing E2E tests against Svelte 5 in master. They're not failing, at least not in the way that I'd expect them to. It makes me want to approach this PR with a little bit of caution

I'll put up a PR with the E2E tests; it may make sense to hold off here until then

@mcous
Copy link
Contributor Author

mcous commented May 23, 2024

@sebastianrothe I traced the suspicious E2E test passing to a build issue! I appreciate the CI runs for #284, they were helpful for me to figure it out. I'm confident now that Svelte 5 modules are not currently working in svelte-jester, which is what I expect!

I think this PR is good to move forward, with the caveat that I don't have much experience using the Svelte compiler itself, so getting more eyes / testing on this might be prudent. It probably makes sense to land #284 first so that the E2E tests can be turned on in this PR

@sebastianrothe sebastianrothe merged commit 40332ca into svelteness:master Jun 10, 2024
6 checks passed
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

Successfully merging this pull request may close these issues.

2 participants