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: v2 migration helpers #11199

Merged
merged 25 commits into from
Dec 11, 2023
Merged

feat: v2 migration helpers #11199

merged 25 commits into from
Dec 11, 2023

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Dec 5, 2023

  • svelte-migrate stuff
  • migration docs

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Copy link

changeset-bot bot commented Dec 5, 2023

⚠️ No Changeset found

Latest commit: 5f6b078

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@benmccann
Copy link
Member

I think we were a little inconsistent in the language of some of the changesets. Maybe we can align them as part of this. Especially .changeset/gentle-guests-clean.md where I said "drop support for Svelte 3" I think maybe we could word in a more upbeat way like "require Svelte 4 to ease the path for Svelte 5 migration" or something.

@benmccann
Copy link
Member

I think it'd be nice for this to appear before "Migrating from Sapper" in the sidebar since it will be relevant to more users. And maybe we can call it "Migrating to SvelteKit v2" rather than "Migration to SvelteKit v2" for consistency, so that both entries begin with "Migrating"

}

/** @param {string} content */
export function update_tsconfig_content(content) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also make sure typescript is on v5 and that moduleResolution is bundler for applications and NodeNext or Node16 for libraries?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK NodeNext is already used for libs for a while already, but we can set bundler if we see nothing is set, or better yet remove the setting entirely.
Typescript should already be ^5.0 through the Svelte 4 upgrade

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. One thing to note is that the v2 branch sets it to NodeNext when running package, which isn't done on master, but it's kind of weird because it might not match what's in the tsconfig.json. Maybe we should have package check if that's the value contained in the tsconfig.json?

@dummdidumm
Copy link
Member Author

Merging now and doing the other changes in their respective PRs sounds good.
Things I have in mind for migration that are part of already merged PRs, which could maybe added here before merging:

  • add path to cookies methods where needed/possible
  • more tsconfig tweaks like removing the moduleresolution and module if it's outdated settings (node)
  • remove dangerZone from svelte.config.js

@Rich-Harris
Copy link
Member

Am trying to insert // @migration task: Specify path comments and I'm just totally lost. Seems like a simple enough thing to do but I can't see any APIs for prepending text, only replacing (which fucks up indentation). Frankly I suspect we'd have a much better time if we ditched ts-morph in favour of using the same traversal techniques we use in svelte (i.e. using acorn with acorn-typescript to generate an estree AST which is much nicer to deal with than TypeScript insanity).

I've wasted enough time fighting with it and there are more important things to do, so I'm just doing to check in my work so far and merge this PR. Those other changes can be follow-ups.

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.

4 participants