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

TypeScript setup #200

Closed
benmccann opened this issue Nov 30, 2020 · 5 comments
Closed

TypeScript setup #200

benmccann opened this issue Nov 30, 2020 · 5 comments

Comments

@benmccann
Copy link
Member

benmccann commented Nov 30, 2020

I'm leaving some notes here for when we eventually take a stab at adding back TypeScript.

Issues we encountered

We had originally tried getting @rollup/plugin-typescript working, but encountered a couple issues:

We instead setup rollup-plugin-typescript2, which was really easy to get setup. However, we eventually did hit one issue:

@rollup/plugin-typescript is super finicky and really hard to get working. rollup-plugin-typescript2 was really easy to setup though it seems not to support pnpm yet and swallows error messages as a result. Normally I would think the best place to focus my efforts would be to fix the one issue in rollup-plugin-typescript2 and then eventually re-enabled it. However, it turns out that rollup-plugin-typescript2 does not have any tests! So in the long-run perhaps it's better to put more work into @rollup/plugin-typescript even though it might be further from working really well

The other issue we were hitting was that the project wasn't being compiled on the CI, so we had a few PRs that broke things. That seems easily avoidable and basically fixed at this point

Suggested solution

I wonder if a lot of our issues may be avoided by just calling tsc directly and removing Rollup from our build process given that the Rollup TypeScript plugins are so finicky. It certainly seems like it would avoid an extra layer of potential complication. TypeScript doesn't yet support npm's new exports map, which means we'll have to continue outputting files to the subpackage root directory instead of using something like a dist/ directory (microsoft/TypeScript#33079)

@benmccann
Copy link
Member Author

I've pushed a branch to use tsc without rollup for when we want to try again: https://github.com/sveltejs/kit/tree/typescript-no-rollup

I started from back when the code was using TypeScript and changed just the app-utils project. It should give a good idea of how the code could be setup. Perhaps we could try changing just one of the sub-packages such as app-utils at some point when we're ready and make sure it doesn't cause any issues for a good while before converting the others over. That should give us some confidence without a lot of code churn and would make it easier to rip it out again if necessary (I would be surprised if we encounter any issues after the Rollup TypeScript plugins have been removed, but still smaller changes are always easier and less risky)

@benmccann
Copy link
Member Author

benmccann commented Dec 1, 2020

@pngwn felt that we would want to bundle intermediate packages and thus keep Rollup in the loop

Pros of bundling:

  • Most control over the dependencies: You will not get in the situation where some patch release of a dependency might break people which you cannot notice because it worked with the older version when you released
  • Might be faster/less bandwidth used
  • Can build just once to create two output formats (e.g. .js and .mjs)

Cons of bundling:

  • Might be downloading things twice (once bundled within your package, once as part of another package)
  • An extra layer of configuration that adds complication

His suggestion was to use rollup-plugin-dts to generate the .d.ts files and then do typechecking with tsc separately

@pngwn
Copy link
Member

pngwn commented Dec 1, 2020

rollup-plugin-dts also 'bundles' types into a single file as well. It definitely works well for first-party types. Bundling third party types is generally pretty error-prone, so I would avoid that where possible. I've found the plugin very pleasant to use in general.

@benmccann
Copy link
Member Author

I was able to get a fix in for rollup/plugins#247. rollup/plugins#287 is much less of an issue, so shouldn't be a blocker. It's easy to search on Google in order to find the workaround of specifying rootDir. I'm hopeful it will eventually get resolved as well. I tracked it down to an issue in TypeScript and filed an issue there, which has been assigned to someone

I've setup a branch that uses @rollup/plugin-typescript here: https://github.com/sveltejs/kit/tree/ts1. Only app-utils is using it at the moment, but I'll update the other packages as well once a new release of @rollup/plugin-typescript is published. This seems like a pretty good path forward, which should solve the issues we were facing earlier, so I'll close this

@ecstrema
Copy link
Contributor

ecstrema commented Feb 24, 2022

Out of curiosity, I just tried @rollup/plugin-typescript and it works fine.

No configuration necessary. See this branch and particularly the last commit. The first two files is all the config that was needed. The rest is me trying it with src/core/config.

The good thing is that it's easy to adopt incrementally.

This commits only sets up typescript for the kit package.

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

No branches or pull requests

3 participants