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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SWC to transpile typescript projects #9049

Closed
wants to merge 4 commits into from

Conversation

dmattia
Copy link
Contributor

@dmattia dmattia commented Feb 25, 2022

Description

I just went down an interesting rabbit hole following the history of the run.ts file that sets up the ts-node running of typescript pulumi programs, and believe that simplifying the existing code will better future-proof pulumi, while providing better performance, with lower maintenance, while supporting more Typescript setups.

Here's the history of this file as I understand it:

This PR does a few things:

  • Removes some of the patching that has happened over time (while still supporting custom tsconfig.json locations that are not in the parent chain of the project dir in the same way as before).
  • Bumps up the ts-node version to major version 10
  • Turns off typechecking
  • Turns on transpiling typescript using SWC, which is sort of like esbuild (https://typestrong.org/ts-node/docs/transpilers/)

I created a test repo that uses these changes, comparing the performance on a plain pulumi new aws-typescript repo using the latest pulumi/pulumi and my fork: https://github.com/dmattia/pulumi-fork-perf-test

Ignoring the CI setup time and yarn installation time, the pulumi preview step with pulumi v3.25.0 took:

and using the fork took:

This is a significant performance increase, and I would encourage anyone interested to fork that repo and test for themselves on more operating systems, node versions, etc. if desired. Or, feel free to comment here what systems you're interested in and I will gladly update the job matrix to test using those options.

As for the lack of typechecking, this is where it gets a lot more into a matter of opinion, but my thought is this: If pulumi wants to typecheck by default, it should use standard typescript handling without custom handling of tsconfig.json files. Doing both of those things by default makes for a challenging onboarding experience for anyone in a custom-setup typescript repo that is not using pulumi new and doesn't just happen to have their tsconfig file in the correct place.

There is also a middle ground here where we do not use the swc transpiler, turn on typechecking by default, and use scope and scopeDir in ts-node to tell typescript to only typecheck the files in the pulumi working directory (see: https://github.com/TypeStrong/ts-node/blob/main/src/index.ts#L209-L218). These options, only available in ts-node v10.x and higher, would make the case where someone had a rogue tsconfig.json file in their home directory a non-issue (although they should really remove that 馃槵) as only the entry file (and other files in the same dir) would be typechecked.

Fixes #4876

Suggested reviewers: @iwahbe, @lukehoban, as they have been involved in many of the above discussions

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@lukehoban
Copy link
Member

lukehoban commented Feb 25, 2022

Nice! The performance improvements look great here.

Turns off typechecking

This is the one biggest question I have here. This is a meaningful user experience change. Today, users get errors early in their deployment if there are TypeScript-level issues. With this change, they would not. We'd be relying on IDE tooling to capture errors before running pulumi up. That may be worth it for the performance win, but there is a bit of a tradeoff here. I think at the very least we should let users opt in/out of this in case they want the alternative to the default.

Removes some of the patching that has happened over time (while still supporting custom tsconfig.json locations that are not in the parent chain of the project dir in the same way as before).

Previously there were "good defaults" for the case where there was not a tsconfig.json - what happens with these changes to the defaults being used for compilation? Do we still support projects with no tsconfig.json? What default compilation options get used?

@Frassle
Copy link
Member

Frassle commented Feb 25, 2022

We've only just recently added the PULUMI_NODEJS_TRANSPILE_ONLY option, echoing Lukes comments I see no reason to remove that option as part of this PR.

@michaelfarrell76
Copy link

This is the one biggest question I have here. This is a meaningful user experience change. Today, users get errors early in their deployment if there are TypeScript-level issues. With this change, they would not. We'd be relying on IDE tooling to capture errors before running pulumi up. That may be worth it for the performance win, but there is a bit of a tradeoff here. I think at the very least we should let users opt in/out of this in case they want the alternative to the default.

@lukehoban maybe there could be an option to include or not include this? We do a similar thing with webpack. disabling type checking during compilation makes a major different in speed. and most of the time the build step doesnt even run on a CI if the type checking fails

@dmattia
Copy link
Contributor Author

dmattia commented Feb 25, 2022

We've only just recently added the PULUMI_NODEJS_TRANSPILE_ONLY option, echoing Lukes comments I see no reason to remove that option as part of this PR.

Yeah I kinda wish I'd waited on merging that PR until I went through the full history here 馃槵

But for sure, I can update to make transpiling optional. So long as standard tsconfig.json setups are supported, that seems fine to me.

We'd be relying on IDE tooling to capture errors before running pulumi up

This is correct, with the additional comment that users can run tsc directly in addition to relying on IDE tooling.

I think at the very least we should let users opt in/out of this in case they want the alternative to the default

Totally fair I think. Do you have a preference on which is the default? Maybe if we want to have typechecking off by default, we leave it as on by default in this PR, and then change over the default during the next major version release?

Previously there were "good defaults" for the case where there was not a tsconfig.json - what happens with these changes to the defaults being used for compilation? Do we still support projects with no tsconfig.json? What default compilation options get used?

I believe this section of ts-node's documentation covers this: https://github.com/TypeStrong/ts-node#tsconfigbases, where ts-node will selectively pick compiler option defaults based on the runtime being used, such as https://github.com/tsconfig/bases/blob/main/bases/node14.json for node14.

I have not tested this fully though other than reading those docs.

@Frassle
Copy link
Member

Frassle commented Feb 25, 2022

Totally fair I think. Do you have a preference on which is the default?

I think we'd want to keep the current behavior of typechecking by default. If you can think of a good way we could better advertise the PULUMI_NODEJS_TRANSPILE_ONLY option to those who could make use it we'd be all ears.

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@dmattia
Copy link
Contributor Author

dmattia commented Feb 25, 2022

@Frassle Makes sense, I'll leave the default as it was then.

I just pushed up the code to make transpilation optional. You can see https://github.com/dmattia/pulumi-fork-perf-test/actions/runs/1900613729 for the new CI tests. If you look into the results, you can see that the time to run pulumi preview with three trials on each setup was:

Using pulumi v3.25.0, PULUMI_NODEJS_TRANSPILE_ONLY=true: 7s, 8s, 9s
Using pulumi v3.25.0, PULUMI_NODEJS_TRANSPILE_ONLY=false: 20s, 21s, 18s
Using this branch, PULUMI_NODEJS_TRANSPILE_ONLY=true: 7s, 7s, 8s
Using this branch, PULUMI_NODEJS_TRANSPILE_ONLY=false: 17s, 21s, 20s

From this, we can see that even with the ts-node v10, this branch should mantain at least even performance with previous branches. Of note, I would expect that SWC would show it's strength more with larger projects, which I have verified in my company's monorepo, but providing test cases for that is a bit trickier.

sdk/nodejs/cmd/run/run.ts Outdated Show resolved Hide resolved
@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@dmattia dmattia requested a review from Frassle February 28, 2022 16:09

// We provide reasonable defaults for many ts options, meaning you don't need to have a tsconfig.json present
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this needs to be saved for a major change version. There are potentially users who currently use pulumi with no tsconfig.json present and so get the default compiler options we currently set. With this change we are instead going to get ts-nodes default compiler options which I suppose is potentially breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I think #8199 is on the docket for release during the next major release as well, and the changes should build nicely off of each other.

@iwahbe iwahbe mentioned this pull request Mar 8, 2022
18 tasks
@dmattia
Copy link
Contributor Author

dmattia commented Jul 13, 2022

This can be potentially closed in favor of #10122 now that newer versions of ts-node support swc as a built-in option: https://github.com/TypeStrong/ts-node/releases/tag/v10.5.0

@julienp
Copy link
Contributor

julienp commented Apr 17, 2024

ts-node and typescript are now optional peer dependencies. The npm package now includes bundled versions of these to fallback to if no peer dependencies are installed. If you add your own versions, these will be prioritised over the bundled versions.

#15622

See also https://www.pulumi.com/blog/typescript-versions/

@julienp julienp closed this Apr 17, 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.

Revisit ts-node version
5 participants