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

fix: do not force slots to be self-closed #435

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Apr 11, 2024

Should enable sveltejs/kit#12102.

@jrmajor jrmajor changed the title breaking: do not force slots to be self-closed fix: do not force slots to be self-closed Apr 11, 2024
@jrmajor jrmajor marked this pull request as ready for review April 11, 2024 20:32
@dummdidumm
Copy link
Member

Thanks for tackling this. My idea was to do these things only if we detect it's Svelte version 5 and above. That makes it non-breaking

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 11, 2024

@dummdidumm I've already updated it to be non-breaking (hence change of PR title). With these changes, both <slot /> and <slot></slot> syntaxes are allowed, so I think this can be released even in a patch release.

For users without the strict setting, this is a bugfix, because <slot></slot> shouldn't be changed in this mode (as far as I understand), and for these with strict setting enabled, it will ignore cases that would be fixed before, which is a slight regression, but it won't break their workflows.

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 11, 2024

BTW, this is only a quick fix to unblock sveltejs/kit#12102. I was planning to follow up with a PR that would force the correct formatting for Svelte 5. Let me know a) whether a PR is welcome b) if it should detect Svelte 5 automatically, or whether it can just be done in a new major version, which seems much simpler.

@Rich-Harris
Copy link
Member

@jrmajor definitely welcome, thank you! A new major seems like probably the best option

@dummdidumm
Copy link
Member

Strictly speaking self-closing slots should be allowed if this is not a custom element, because the slot is not really the html slot in that case. It looks the same but it's something different. Don't think we need to disallow that then. The good think is that this should be detectable because svelte:options needs to be present in custom element components declaring them as such.

As for major or detect Svelte 5: Major is definitely simpler but I need to think some more about other breaking changes we may want to make. If we do major, we should also use the new AST (parse(code, { modern: true }))

@dummdidumm dummdidumm merged commit 539169b into sveltejs:master Apr 12, 2024
1 check passed
@jrmajor jrmajor mentioned this pull request Apr 19, 2024
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.

None yet

3 participants