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: ensure migrate keeps inline/trailing comments in $props type definition and produces non-broken code #14143

Merged
merged 7 commits into from
Nov 4, 2024

Conversation

rgon
Copy link
Contributor

@rgon rgon commented Nov 4, 2024

Fixes #14140

Keeps inline comments when migrating types, as we already do with other statement types L783

    export let step: number | null = null // In-line comments are now kept
    interface Props {
	step: number | null; // In-line comments are now kept
    }

Added tests for Typescript and JSDoc migration. In the latter, the comments are merged with leading comments. See tests/migrate/jsdoc-with-comments/output.svelte

Copy link

changeset-bot bot commented Nov 4, 2024

🦋 Changeset detected

Latest commit: e00d318

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rgon rgon changed the title feat: migrate keep inline/trailing comments in $props type definition fix: ensure migrate keeps inline/trailing comments in $props type definition Nov 4, 2024
@dummdidumm
Copy link
Member

Thank you! Could you add a changeset? Then we're good to merge

Copy link

pkg-pr-new bot commented Nov 4, 2024

pnpm add https://pkg.pr.new/svelte@14143

commit: e00d318

@rgon
Copy link
Contributor Author

rgon commented Nov 4, 2024

Changeset added!

@paoloricciuti
Copy link
Member

Just one comment...while a bit weird unfortunately you can't assume trailing comments are only line comment. For example this

<script>
	export let name = 'world'; /*
	* this is a trailing comment too
	**/
</script>

migrate to broken code

<script>
	/**
	 * @typedef {Object} Props
	 * @property {string} [name] - * this is a trailing comment too
*
	 */

	/** @type {Props} */
	let { name = 'world' } = $props();
	* this is a trailing comment too
	**/
</script>

although to be fair it's broken in today migration too...but the comment should probably undergo the same "cleaning" process we apply to the leadingComments

@rgon
Copy link
Contributor Author

rgon commented Nov 4, 2024

@paoloricciuti Nice catch! Absolutely fair to fix it while we're at it and add tests, especially when it produces broken code.

I have modified this PR with the fix (and reworked comment parsing a bit so that it's exactly like leadingComments) and added tests for the use case you mentioned.

Adding a new patch fix to the changeset as well, since the issue predates my PR (and arguably is more important than the original issue!).

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14143-svelte.vercel.app/

this is an automated message

@rgon rgon changed the title fix: ensure migrate keeps inline/trailing comments in $props type definition fix: ensure migrate keeps inline/trailing comments in $props type definition and produces non-broken code Nov 4, 2024
@paoloricciuti
Copy link
Member

@rgon if you can fix linting we are good to merge

@paoloricciuti
Copy link
Member

The linting issues aren't in any of my touched files:

It was a small issue in the migrate file...let's wait on all greens and i'll merge

@rgon
Copy link
Contributor Author

rgon commented Nov 4, 2024

@paoloricciuti thanks, I got a bit confused with the sites/svelte-5-preview warnings when running lint on the entire pnpm workspace, since it runs on parallel and merges the log. Was about to push a fix

@paoloricciuti paoloricciuti merged commit e6dd871 into sveltejs:main Nov 4, 2024
10 checks passed
@paoloricciuti
Copy link
Member

Thanks! 🚀

@github-actions github-actions bot mentioned this pull request Nov 4, 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.

sv migrate svelte-5 drops comments
4 participants