Skip to content

perf(transformer): remove createProgram from transpiler #1941

Merged
ahnpnl merged 1 commit intokulshekhar:masterfrom
zhming0:remove-program-from-transpiler
Sep 11, 2020
Merged

perf(transformer): remove createProgram from transpiler #1941
ahnpnl merged 1 commit intokulshekhar:masterfrom
zhming0:remove-program-from-transpiler

Conversation

@zhming0
Copy link
Copy Markdown
Contributor

@zhming0 zhming0 commented Sep 11, 2020

Summary

I found that Typescript's createProgram API is called even when isolatedModule is set.
This is a major slow down (100 msecs vs 10~50 secs) for test start up and it doesn't seem to make sense for isolatedModule anyway.
Longer test start up -> longer feedback cycle -> lower productivity.

Test plan

N/A. The program isn't used by any part of ts-jest, at least not in its critical path.

Does this PR introduce a breaking change?

  • Yes
  • No
  • Maybe

I just don't know enough about the use case of program for isolatedModule.

Other information

I tried to gather the history around this but couldn't find any notes regarding transpiler's need for program.
If the createProgram turned out to be desirable thing here, I am happy to create another PR to introduce a flag to disable it.

Otherwise, I believe majority of isolatedModule users will benefit from this PR greatly.

@zhming0 zhming0 requested a review from kulshekhar as a code owner September 11, 2020 12:27
@zhming0 zhming0 force-pushed the remove-program-from-transpiler branch from 21dd0aa to c8f32bc Compare September 11, 2020 12:28
@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Sep 11, 2020

this is strange, unless something call program.emit() will trigger something, I didn't notice there is downgrade performance with isolatedModules: true when createProgram is called.

The reason why createProgram is there because in custom AST transformers, some might want to access to the internal Program that ts-jest is using. However, since transpile API is used for isolatedModules: true, createProgram might not make any sense.

I agree that we can remove it.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 11, 2020

Pull Request Test Coverage Report for Build 5859

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 93.367%

Totals Coverage Status
Change from base Build 5857: -0.003%
Covered Lines: 1132
Relevant Lines: 1166

💛 - Coveralls

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