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

perf(transformer): remove createProgram from transpiler #1941

Merged
merged 1 commit into from Sep 11, 2020

Conversation

zhming0
Copy link
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 force-pushed the remove-program-from-transpiler branch from 21dd0aa to c8f32bc Compare September 11, 2020 12:28
@ahnpnl
Copy link
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

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

@ahnpnl ahnpnl merged commit dd84534 into kulshekhar:master Sep 11, 2020
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.

None yet

3 participants