Skip to content

Conversation

@bluwy
Copy link
Member

@bluwy bluwy commented Nov 8, 2023

While fixing issues to support Vite 5, the types moduleResolution setup needs to be updated to use node16 to work. I also aligned the existing tsconfig setup so it's consistent.

NOTE: This only fixes the typechecking issues with Vite 5, there still seems to be test fails with it that I'll look into separately.

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:.

@changeset-bot
Copy link

changeset-bot bot commented Nov 8, 2023

⚠️ No Changeset found

Latest commit: 0843ca0

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

We've avoided updating to TypeScript 5 (and thus dts-buddy as well) to avoid any breaking changes. Hopefully we can do it soon with SvelteKit 2, but can we skip that part for now?

@bluwy
Copy link
Member Author

bluwy commented Nov 9, 2023

How would the TS version we use affect the users? I thought it's a dev dep we can choose ourselves 🤔

I can certainly try to avoid bumping typescript and dts-buddy but it'll be a bit tricky.

@benmccann
Copy link
Member

If we use Typescript 5 then people can start using newer language features that didn't exist in Typescript 4

I haven't checked yet if dts-buddy actually requires Typescript 5. Perhaps we could set it's peer range to include 4 as well

@bluwy
Copy link
Member Author

bluwy commented Nov 9, 2023

Hmm I'm not really sure we should hold ourselves back for that. We could create a separate tsc check using TS 4 if we want to, but the types concern hasn't really been an issue in my (outside) experience.

But if we prefer to not bump it, I can keep that and find some middleground fix later tonight.

@benmccann
Copy link
Member

If we wanted to have a tsc check for TS 4 support would we need to check every individual type?

@bluwy
Copy link
Member Author

bluwy commented Nov 9, 2023

I reverted the deps upgrade, and added a types fix to support Vite 5 so typecheck passes in vite-ecosystem-ci. 133ee43

I had to use --skipLibCheck for some packages as moduleResolution: node16 causes issues for some deps (added comments), and use --skipLibCheck for test apps since we can't use moduleResolution: bundler (TS 5). It's not really ideal as it skips typechecking the projects' .d.ts files too, but happy to hear any alternatives.


There still seems to be test fails in Vite 5 (typechecking aside). I think I'll look into that in a separate PR.

@bluwy
Copy link
Member Author

bluwy commented Nov 9, 2023

Looked at the test fail, looks like it's also related to types. It's failing here.

Can't really fix that with skipLibCheck otherwise the test wouldn't have been useful. We'd need TS 5 and moduleResolution: bundler to fix it. Maybe that might have to be skipped temporarily for Vite 5 only.

@benmccann
Copy link
Member

ooh, wait. I just noticed something. don't merge yet!

@bluwy
Copy link
Member Author

bluwy commented Nov 9, 2023

Thanks for cleaning up the PR. I'm not sure why you removed the skips though, I noted at #10997 (comment) why it's needed. Though also understandable if you want to handle this when we upgrade TS and support Vite 5 later?

@benmccann
Copy link
Member

I'm afraid I still don't understand why you had the skips. I'm not getting any errors without them. What do you need to do to see errors without them present?

@bluwy
Copy link
Member Author

bluwy commented Nov 10, 2023

You can link Vite 5 locally, and run pnpm check to see the fails. Vite 5 re-exports rollup/parseAst which only works when TypeScript respect the "exports" field. Currently the tests aren't able to be configured to respect "exports" due to the lower TS version, so figured it was easier to skip it.

@benmccann
Copy link
Member

#10818 was passing lint checks without the skips. Maybe that's because it's on an older beta? If it ends up failing we can add it back as part of that PR. CC @dominikg

@bluwy bluwy mentioned this pull request Nov 12, 2023
5 tasks
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.

3 participants