search

sveltejs / prettier-plugin-svelte

Format your svelte components using prettier.
MIT License
714 stars 95 forks source link

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

Closed jrmajor closed 2 months ago

jrmajor commented 2 months ago

Should enable sveltejs/kit#12102.

dummdidumm commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

dummdidumm commented 2 months ago

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 }))