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

buildSolutionReferences with instance transformers #1274

Conversation

gasnier
Copy link
Contributor

@gasnier gasnier commented Mar 27, 2021

Fixes #1025

I first tried a solution inspired from https://stackoverflow.com/questions/62026189/typescript-custom-transformers-with-ts-createwatchprogram/62132983#62132983
Where I used getNextInvalidatedProject from compiler api that is defined here :
https://github.com/microsoft/TypeScript/blob/master/src/compiler/tsbuildPublic.ts
This worked for standalone builds but was not working in watch mode.
In watch mode, it is SolutionBuilder that handles the emit and today there is no way to pass customTransformers to SolutionBuilder.

The final solution overrides the emit function and is based on https://github.com/nonara/ts-patch

@gasnier gasnier marked this pull request as draft March 28, 2021 10:55
@johnnyreilly
Copy link
Member

johnnyreilly commented Mar 28, 2021

Heads up, we've just renamed from master to main - https://twitter.com/johnny_reilly/status/1376156935414820869

This will have updated this PR but not your local branch. To apply there then:

git branch -m master main
git fetch origin
git branch -u origin/main main
git remote set-head origin -a

@gasnier gasnier marked this pull request as ready for review April 19, 2021 09:48
@gasnier
Copy link
Contributor Author

gasnier commented Apr 19, 2021

@johnnyreilly
This PR is ready for review

@johnnyreilly
Copy link
Member

Thanks - I'll try and take a look this weekend

@johnnyreilly
Copy link
Member

Sorry - I forgot about this. Just updated the branch. Will take a look.

@johnnyreilly
Copy link
Member

Is transformers on instance now unused? If so, should it be removed?

https://github.com/gasnier/ts-loader/blob/6bb61d4088fcae03feca700e59f6e96c77f6244c/src/interfaces.ts#L212

src/transformers.ts Outdated Show resolved Hide resolved
src/transformers.ts Outdated Show resolved Hide resolved
@johnnyreilly
Copy link
Member

Looking good I think. Two things:

  • this PR is effectively working around the fact there is no way to pass customTransformers to SolutionBuilder. It would be good to raise that absence against the TypeScript project itself so they are aware of this. Perhaps in future a different approach could be used if something is implemented in TypeScript. Could you either link to the issue if it already exists, or raise it and link to it? Thanks. cc @sheetalkamat
  • could you update package.json and CHANGELOG.md? Thanks!

customTransformers?: ts.CustomTransformers
): ts.EmitResult {
/* Invoke TS emit */
const result: ts.EmitResult = originalEmit(
Copy link
Contributor

Choose a reason for hiding this comment

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

This will have some bad effects .. emit is used for emitting dts files to determine shape of the file as well and take internal parameters and that signature is internal to typescript .. also I don’t think you want transformers for that reason ..

I have been thinking about your issue and was wondering if build could take getTransformers as function that returns transformers based on project path.. haven’t had chance to investigate that part yet but this approach is definitely not correct

Copy link
Member

Choose a reason for hiding this comment

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

Oh thanks for that @sheetalkamat - very helpful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if we can pass customTransformers to build in SolutionBuilder, this solves the problem.
the done function on invalidatedProject already has the customTransformers parameter.
image

A function by projet path is ok, anyway in ts-loader there is only one global configuration so we would always pass the same transformers. A good solution would be to do like ttypescript (https://github.com/cevek/ttypescript) and read the configuration from tsconfig.json

Copy link
Contributor

Choose a reason for hiding this comment

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

microsoft/TypeScript#43984 add the typescript side of the API

@johnnyreilly
Copy link
Member

Should we close this now?

@gasnier
Copy link
Contributor Author

gasnier commented May 29, 2021

Yes, I close it, I didn't know Typescript 4.3.1 was out...
Thanks @sheetalkamat

I will create another issue / PR to solve this using SolutionBuilder.

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.

Custom transformers not working for referenced projects
3 participants