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

Migration tool feedback #6059

Closed
benmccann opened this issue Aug 19, 2022 · 2 comments · Fixed by #6070
Closed

Migration tool feedback #6059

benmccann opened this issue Aug 19, 2022 · 2 comments · Fixed by #6070

Comments

@benmccann
Copy link
Member

benmccann commented Aug 19, 2022

Describe the bug

Three issues I noticed that may be able to be automatically migrated:

Reproduction

noticed when working on sveltejs/sites#371

Logs

No response

System Info

n/a

Severity

annoyance

Additional Information

No response

@dummdidumm
Copy link
Member

fetch wasn't added in load so I ended up using global fetch accidentally. not the worst though because I got a console warning about that

Where did that happen specifically? AFAIK nothing is removed/added to the load function, so this was likely an existing bug

it could probably migrate /** @type {import('@sveltejs/kit').PageLoad} / to /* @type {import('./$types').PageLoad} */

I'm hesitant to do this, because people could have a reason for this. We don't know if they use TypeScript at all (or rather, have TypeScript in their package.json) in this case; if they don't the $types aren't generated.

dummdidumm added a commit that referenced this issue Aug 19, 2022
Rich-Harris pushed a commit that referenced this issue Aug 19, 2022
@benmccann
Copy link
Member Author

The fetch thing happened several places. It looks like you're right about it being an existing bug: https://github.com/sveltejs/sites/blob/1c83a81d3957812a2986c6c50667d3cfca790980/sites/svelte.dev/src/routes/examples/__layout.svelte#L5

I'm not sure I understand what you were suggesting about the types migration. I assumed if they already had a type declaration then they were using type checking, but it sounds like they could have that comment without TypeScript installed which would cause issues if we were to automatically migrate?

Thanks for fixing the typo and reviewing this issue!

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 a pull request may close this issue.

2 participants