getCustomTransformer program mismatch

[alex-dixon]

Not sure how to fit into the test framework yet.

 yarn run comparison-tests -- --single-test customTransformer 

Should print out a list of files for non-transpileOnly, empty array for transpile only. Expected a list of files for both.

[alex-dixon]

Tests passing locally.

When transpileOnly is enabled, a ts.Program is now instantiated with the same parameters from the config for non-transpileOnly mode, except the transpileOnly program is instantiated only with files that fs.existsSync returns truthy for. This honors the existing behavior in the tsconfigInvalidFile test, where transpile only should not emit errors for files that do not exist.

Formerly the transpileOnly program was instantiated with an empty array. I'm not sure why. Downstream consumers, like transformers returned by getCustomTransfomers, received a ts.Program with no source files. They would receive them when transpileOnly was false.

[johnnyreilly]

I think this may undo the work of @Zn4rK in

#1352

[alex-dixon]

Sorry. Wasn't intentional just didn't see it but...Wouldn't const getProgram = () => program just return the same reference that was passed in originally?

Let me know, can add it back.

[Zn4rK]

@alex-dixon It would, but the idea is to eventually remove the passing of the reference completely, and use a function instead, so we're consistent over the different ways a program can be created/referenced.

[johnnyreilly]

Is this good to review now?

[alex-dixon]

@johnnyreilly ready for review

[johnnyreilly]

Looks good. Do you want to update the CHANGELOG.md and package.json

[Zn4rK]

I've been thinking about this a lot the past couple of days. I personally like this change, but for people already using transpileOnly, the initial compilation will be longer, since a real program, with real source files needs to be created. TypeScript will resolve all the relations between the files.

I think that the reason for the empty program is because with the flag, a "real" program doesn't need to be created.

[alex-dixon]

@Zn4rK Not totally following. :)

getCustomTransformers requires type-level/AST information in all cases in order to be useful (I'm assuming). Right now it can get that via ts.SourceFiles (via before(),after() invocations). That holds both for transpileOnly and not. Where do the ts.SourceFiles come from? I was assuming a ts.Program (with source files).

[Zn4rK]

Formerly the transpileOnly program was instantiated with an empty array. I'm not sure why. Downstream consumers, like transformers returned by getCustomTransfomers, received a ts.Program with no source files. They would receive them when transpileOnly was false.

@alex-dixon I'm arguing that the reason that an empty program was instantiated was because that transpileOnly doesn't need a real program to just transpile ts to js. This behaviour is very unexpected in the context of getCustomTransformers, but not for regular usage.

With this change a real program is created, even though we don't need it for just transpilation. getCustomTransformers does get an empty program, but get the updated sourcefiles via ts.TransformerFactory.

Ideally we could merge this change, but what I'm saying is that we probably need to some profiling to verify that we don't increase the initial compilation time.

Alternatively, a non-empty program could be created conditionally if getCustomTransformers is present in the options?

[alex-dixon]

@Zn4rK

Alternatively, a non-empty program could be created conditionally if getCustomTransformers is present in the options?

How do we feel about this after a few weeks? 😊 I don’t love it but it will solve the issue and other solutions I’ve thought about would require more discussion and would be more significant changes.

I’ve come across a couple SO posts and the like where people have been bitten by an empty program with transpileOnly enabled. Some kind of change here seems worthwhile.

The only lingering thought is that the perf hit could be significant and I don’t fully know how people are using getCustomTransformers.

If it’s worth further work to allow users to opt in to that cost, the hit could be paid on the first invocation of getProgram. Existing code that just uses the program argument would see no change.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

Closing as stale. Please reopen if you'd like to work on this further.