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

Avoid importing typescript unless necessary #10185

Merged
merged 10 commits into from
Jul 20, 2022

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Jul 19, 2022

Description

The key motivation for this change is speeding up startup of small JavaScript programs. That is:

pulumi new javascript

# edit Pulumi.yaml to have:
# runtime:
#  name: nodejs
#  options:
#    typescript: false

time pulumi preview
# before : 1.9s
# after: 1.6s
# 300ms gained or 15%

This 300ms is simply is the cost of adding ts-node and typescript dependency to the imports.

Importing "runtime" directly considered harmful because it imports "typescript" indirectly via "closure" support.

Fixes # (issue)

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

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 19, 2022

@RobbieMcKinstry brings up a question whether lazy-load technique taken from https://www.typescriptlang.org/docs/handbook/modules.html#optional-module-loading-and-other-advanced-loading-scenarios will be supported in all the needed module environments (CommonJS etc).

@t0yv0 t0yv0 requested a review from a team July 19, 2022 20:38
@t0yv0
Copy link
Member Author

t0yv0 commented Jul 19, 2022

Adding platform-team reviewers to seek out node expertise. Also hoping tests might help catch.

@RobbieMcKinstry
Copy link
Contributor

After reviewing https://blog.bitsrc.io/javascript-require-vs-import-47827a361b77 I feel like its likely.

@t0yv0 t0yv0 removed the request for review from a team July 19, 2022 21:54
@t0yv0
Copy link
Member Author

t0yv0 commented Jul 19, 2022

Let me work through failing tests first.

@t0yv0 t0yv0 force-pushed the t0yv0/avoid-importing-typescript branch from 252d9a2 to 31670b1 Compare July 20, 2022 02:00
@t0yv0 t0yv0 added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jul 20, 2022
@Frassle
Copy link
Member

Frassle commented Jul 20, 2022

Importing "runtime" directly considered harmful because it imports "typescript" indirectly via "closure" support.

My current intention is to remove closure in 4.0 and ship it as a separate package.

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 20, 2022

That'd be good but the changes here still help since ts-node imports depend on typescript. I think I'd like to ship this. Concerns I've heard so far was:

  • does it work with all JS module systems (which ones does Pulumi support, how do I test)
  • does it slow down TypeScript projects (will tests)

@t0yv0 t0yv0 requested a review from a team July 20, 2022 12:56
@t0yv0 t0yv0 marked this pull request as ready for review July 20, 2022 12:56
@RobbieMcKinstry
Copy link
Contributor

@t0yv0 After digging into minification, I don't think it will slow down typescript builds, since we don't bundle. If we bundled, then a dynamic import would cause a disk read that otherwise wouldn't have occurred in the static import scenario.

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 20, 2022

Ah so would bundling embed "typescript" and "ts-node" JavaScript code inside the "pulumi" bundle? I thought it's just a matter of whether pulumi specific modules are distributed in one or several files.

With that being clarified it seems good for us not to bundle "typescript" since it is large and is not always used by the Pulumi code, it's optional in some scenarios.

Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

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

This looks good to me - I don't think a delayed require() will be a problem.

@t0yv0 t0yv0 merged commit f6fc099 into master Jul 20, 2022
@pulumi-bot pulumi-bot deleted the t0yv0/avoid-importing-typescript branch July 20, 2022 21:38
@RobbieMcKinstry
Copy link
Contributor

Ah so would bundling embed "typescript" and "ts-node" JavaScript code inside the "pulumi" bundle? I thought it's just a matter of whether pulumi specific modules are distributed in one or several files.

With that being clarified it seems good for us not to bundle "typescript" since it is large and is not always used by the Pulumi code, it's optional in some scenarios.

Yes, if the runtime doesn't use any dynamic imports, then adding bundling would (by default) bundle typescript and ts-node. Which I agree is something we'd like to avoid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants