Skip to content

Conversation

@samuelscheit
Copy link

Also discussed in #85 #86 #96 and microsoft/TypeScript#51493 (comment)

Problem

Currently ts-patch patches the createProgram function of the typescript compiler to transform the program.
Additionally it patches the emit function of the program to transform emitted files.
The problem is that the Typescript language service creates the program at startup and then adds the files to the program incrementally, resulting in the files not being handled by createProgram/emit.
This leads to an inconsistent state between the tsc compiler and the ts language service, since the editor may display errors of the program that would not exist if the program had been transformed before.

Solution

Additionally to patching the createProgram and emit function, ts-patch also patches the parseSourceFile function, which gets called for every single file, after it has been loaded. This way ts-patch ensures that the transformer plugins have transformed the file, before the editor handles it.
Within the patched parseSourceFile function ts-patch loads the plugins of the tsconfig.json file and executes them after the file has been parsed by the original parseSourceFile function.

I've added a working implementation for this feature, but currently there is no distinction between "source transformer plugins" and "parseSourceFile transformer plugins".
Do you have any suggestions on how to differentiate between the plugins?

@nonara
Copy link
Owner

nonara commented Aug 13, 2023

Hi Samuel.

Thanks for the contribution! I appreciate the willingness to help add to the library.

With that said, I don't I'm not sure that this approach is something we'd want to integrate. If your goal is to remove errors or modify LS behaviour, it would be better to modify the diagnostics arrays or write a language service plugin.

AFAIK, the Language Service isn't built to handle transformed source. When a SourceFile is transformed, it is fundamentally different in a number of ways. For one, the Language Service needs to have exact correlation between line and position information and the actual file you see in your editor. Transformed nodes would not have this.

Even setting that aside, I imagine you'd see some pretty nasty issues if you handed it a post-transform SourceFile. Even if you didn't, though, I'm not sure I understand why you'd want to do it. I feel fairly confident that there is a better approach to solve your issue — would you mind sharing more about what it is you'd like to accomplish?

Overall, I'm going to have to say that transformers are meant for the emit process, and I don't think that they would be useful / functional in the Language Service.

@jakebailey if you had anything to add on this, I'd be interested to hear it! We get these sorts of requests often, and my feeling has always been that this is not viable (utility aside).

Best,
Ron

@jakebailey
Copy link

I might be wrong, but I believe that the language service can in fact emit files automatically if one uses compileOnSave: true in the tsconfig. And since the LS has the program (just a getProgram() call away), one can also just call emit; IIRC ts-morph uses the language service to load files and navigate them, but also does everything else with its program including emit (https://github.com/dsherret/ts-morph/blob/latest/packages/ts-morph/src/ProjectContext.ts). So, I don't think it's unprecedented.

@nonara
Copy link
Owner

nonara commented Aug 13, 2023

@jakebailey I appreciate the input! That does make sense.

As far as emit goes, even from LS, I believe our current logic should already have that covered. (If that's not accurate, @samuelscheit, let me know)

However, I believe that's just in the outflow process during Emit. If I'm recalling correctly, emit creates a transformed copy of the SourceFile, but it wouldn't replace the original in the Program instance. If that's true, that the Language Service's Program would retain the originals, and therefore every Node would have position detail and no artifacts of transformation.

(Edit: Granted, I know originals can be traced out, I just assume that if the LS wasn't built with this in mind, it may not support it out of the box)

The approach that's taken in this PR is to patch parseSourceFile(), which effectively feeds a transformed SourceFile as an input to the Language Service. That's what I'm thinking might cause issues.

I was mainly wondering if there is any precedent there? My guess it that the LS expects all nodes to have position info. Transforming them on the way in seems like it wouldn't be a good approach.

@jakebailey
Copy link

However, I believe that's just in the outflow process during Emit. If I'm recalling correctly, emit creates a transformed copy of the SourceFile, but it wouldn't replace the original in the Program instance. If that's true, that the Language Service's Program would retain the originals, and therefore every Node would have position detail and no artifacts of transformation.

Yes, right; this is purely for emit. Truthfully I didn't check the PR but messing with the tree within a program is likely to cause issues becuase that's needed for editor features, used for locations, etc; But, I have heard of people managing to do it but only by injecting code with a faked zero length such that the later position info is still correct. (Maybe volar?) No clue about incremental mode, though.

@nonara
Copy link
Owner

nonara commented Aug 13, 2023

I appreciate the work, @samuelscheit! I'm going to go ahead and close this, but I'm still happy to discuss what you're trying to accomplish and see if I can help you find a better route.

And thanks to Jake for weighing in!

@nonara nonara closed this Aug 13, 2023
@jakebailey
Copy link

To be clear, I'm not against it if it works, but you definitely want to be using two different kinds of plugins for the two things, somehow. This "post-parse/pre-bind" kind of plugin is one that had been considered on TS itself as a way to provide the checker with code to check (rather than hoping it works out later), but is of course a bit complicated to get correct due to the things you've noted.

@nonara
Copy link
Owner

nonara commented Aug 13, 2023

No worries!

I'm also certainly open to reopening if there's a valid use-case, but at the same time, I think relying upon the Language Service to support a transformed SourceFile is a big ask that we'd have to deeply verify was possible first. Next, we'd have to closely confirm our approach.

More important, though, is the question of why we'd want to do it. I think we'd need a pretty compelling case for that.

For example, if we have an error in transformed code, what are we telling the IDE is the source of the issue? My guess is that the intended use-case here is to change LS diagnostic behaviour, in which case an LS plugin would usually be more appropriate.

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