search

ezolenko / rollup-plugin-typescript2

Rollup plugin for typescript with compiler errors.
MIT License
822 stars 71 forks source link

fix: type-check `include`d files missed by `transform` (type-only files) #345

Closed agilgur5 closed 2 years ago

agilgur5 commented 2 years ago

Summary

Type-check some type-only files that were missed in the transform hook but are in parsedConfig.fileNames

Details

(Note: Technically speaking, there could be full TS files (i.e. not type-only) that are in the include and weren't transformed. These would basically be independent TS files not part of the bundle that the user wanted type-checking and declarations for (as we already generate declarations for those files))

For more details, see my root cause analysis in https://github.com/ezolenko/rollup-plugin-typescript2/issues/298#issuecomment-1146658442

Review Notes

Misc

Could the walkTree call be moved into buildEnd as well? That seems like it might be a better place for it, the !noErrors output, and maybe cache.done() too, but I don't know if there was a reason for putting it in _ongenerate.

agilgur5 commented 2 years ago

@ezolenko this is ready for review now. I double-checked that this worked locally against a similar error to #298's example (still need to get to adding integration tests). If you could respond to the last review note and the "Misc" questions as well, that would be helpful

ezolenko commented 2 years ago

I think the main reason walkTree was in _ongenerate was that there was no buildEnd hook at the moment (or I missed it). That's why there is generateRound variable too, to avoid reprinting errors it for each output target.

If it works in buildEnd it can go there. Main requirement for watch mode is that all errors are printed at the end of each cycle, even for files that were not retranspiled in a given cycle.

If you run in watch mode and break / fix / break / fix various files you would see if this behaves as expected. This applies to return code as well, ideally error caught at that stage behaves like error caught at transpile stage regarding rollup behavior and return code. For example with abortOnError option set, it should print first error and bail out without typechecking anything else (I imagine this option being used when failing must happen as soon as possible).

agilgur5 commented 2 years ago

I think the main reason walkTree was in _ongenerate was that there was no buildEnd hook at the moment (or I missed it). That's why there is generateRound variable too, to avoid reprinting errors it for each output target.

OOOHHHH... that makes so much more sense. It looked like a workaround for buildEnd, but for some reason I didn't suspect that buildEnd literally didn't exist before.

Confirmed in the Rollup changelog that buildEnd was added in 0.60, which this code indeed predates. rpt2 currently sets a min version of Rollup >=1.26.3, so it wouldn't require any version changes to switch to using buildEnd instead now.

I think some of the other legacy checks like _.isFunction(this.error) and if (this.addWatchFile) can be removed now too. I thought they were still necessary for backward-compat, but all those seem to predate even Rollup 1.0.0.

I'll change those in separate PRs though, will keep this one independent and limited in its scope (as I usually do 🙂 )

If you run in watch mode and break / fix / break / fix various files you would see if this behaves as expected.

Let me double-check this, but it should behave like that according to the Rollup spec.

Main requirement for watch mode is that all errors are printed at the end of each cycle, even for files that were not retranspiled in a given cycle.

Oh, is this meant as a workaround for watch mode clearing the terminal by default on a new cycle? I thought the optimal tree walk would only be for files that depend on those that changed. Since transform isn't called on every file again, at least, as far as I understand (and I may have an incorrect understanding of that), so lots of errors still won't be reported or declarations re-generated as a result of that.

This applies to return code as well, ideally error caught at that stage behaves like error caught at transpile stage regarding rollup behavior and return code. For example with abortOnError option set, it should print first error and bail out without typechecking anything else (I imagine this option being used when failing must happen as soon as possible).

So I didn't go into this into that much detail in "Review Notes", but the type-errors here seem to never return emitSkipped (probably because they don't impact the Program except for ambient typings? not sure). And in transform we only bail out when emitSkipped: true. So the current buildEnd code does act in the same way as transform, but tsc doesn't act this way -- which is very confusing -- tsc does exit non-zero, despite emitSkipped: false. Like it outputs declarations, and those declarations don't pass type-check (same as the type-only file itself), so emitSkipped: false, but tsc exits non-zero. It only doesn't emit when noEmitOnError: true (the opposite of the default).

So I could change buildEnd to call this.error, but that behavior is actually slightly different from transform. Like a key difference would be for check: false: there's no way for us to know to bail and attempt type-checking on a fatal error because emitSkipped: false. I.e. the flag for a fatal error itself isn't set by TS. So if we do call this.error in buildEnd, this would mean that it may bail if check: true, but will pass if check: false, even with abortOnError: true in both cases.

That's some weird divergent behavior, so I'm not sure which is optimal. But I can do either, just want to call out those trade-offs explicitly because this is unique to these files.

For example with abortOnError option set, it should print first error and bail out without typechecking anything else (I imagine this option being used when failing must happen as soon as possible).

Kinda unrelated, but this has popped up in the issues pretty often since TS defaults to noEmitOnError: false. Implementing #168 would be ideal so that we can more closely match tsc's behavior, but I'm not sure if that'll always produce valid JS code, since Rollup could throw, which kinda ruins the point of improving DX, as it would be worse DX for Rollup to throw a confusing error. I looked into your old branch for that one and I think it doesn't produce invalid code, and if emitSkipped, we could just return null or something in transform, then bail out in buildEnd instead, once all errors have printed out. So I think it's doable, but not 100% sure on that.

agilgur5 commented 2 years ago

Let me double-check this, but it should behave like that according to the Rollup spec.

Yep, buildEnd behaves as expected.

The only problem I discovered during watch mode is that some errors can get printed twice 😕 . That's because the new buildEnd type-checks and walkTree type-checks as well. And walkTree intentionally doesn't check checkedFiles.

So I'm gonna have to switch the ordering here. Which means I might as well move _ongenerate into buildEnd in this PR

agilgur5 commented 2 years ago

Ah, buildEnd also has one tiny difference from the current methodology -- buildEnd is called even if a build errors out, whereas generateBundle / _ongenerate isn't.

So I need to add a check for the error case and not walkTree if so. But still type-check parsedConfig.fileNames to support #168 (which I think should be the default to match tsc). EDIT: I think I'll simplify this for now and just not type-check more if there's an error, as this is slowly becoming a more complex PR than originally anticipated.

agilgur5 commented 2 years ago

And in transform we only bail out when emitSkipped: true.

Ok, turns out this isn't entirely correct. I thought my understanding of this was a bit off.

Since abortOnError is baked into the RollupContext, a TS diagnostic of DiagnosticCategory.Error will cause it to bail.

But in this portion of code (both the new code I added and the existing walkTree code) we're using ConsoleContext instead of RollupContext, so I think therein lies the problem.

I actually have been wondering about why ConsoleContext vs. RollupContext (which is only used in transform) for a while -- could you clarify that @ezolenko? Should RollupContext be used instead of ConsoleContext in this new code and in the existing walkTree code?

one of the few remaining missing pieces in my current understanding of the codebase

agilgur5 commented 2 years ago

The only problem I discovered during watch mode is that some errors can get printed twice 😕 . That's because the new buildEnd type-checks and walkTree type-checks as well. And walkTree intentionally doesn't check checkedFiles.

So I'm gonna have to switch the ordering here. Which means I might as well move _ongenerate into buildEnd in this PR

Completed moving _ongenerate into buildEnd now and fixing the ordering. While I was at it, basically renamed _onwrite to generateBundle, as now that's the only function called in generateBundle anyway.

Ah, buildEnd also has one tiny difference from the current methodology -- buildEnd is called even if a build errors out, whereas generateBundle / _ongenerate isn't.

Handled the error case as well now. Because the error case is handled here now, I also moved cache().done() from emitSkipped into here as well (since this.error will trigger buildEnd).

Can read the commit messages for more details

But in this portion of code (both the new code I added and the existing walkTree code) we're using ConsoleContext instead of RollupContext, so I think therein lies the problem.

I actually have been wondering about why ConsoleContext vs. RollupContext (which is only used in transform) for a while -- could you clarify that @ezolenko? Should RollupContext be used instead of ConsoleContext in this new code and in the existing walkTree code?

So this is the only portion left remaining for this PR, that I'm actually not sure of myself (as it is historical, as far as I know), so could still use some input on this to finish up this PR

ezolenko commented 2 years ago

The only reason ConsoleContext exists is that PluginContext is not available in some places where we want to print something. And the main behavior difference is that printing error in PluginContext would kill the build , but printing error in ConsoleContext would not, that's why RollupContext has this bail check to downgrade error to warning. There is also some cosmetic output differences I think.

So yeah, just using RollupContext when code moves should be fine I think.

agilgur5 commented 2 years ago

The only reason ConsoleContext exists is that PluginContext is not available in some places where we want to print something.

Yea, that's what I thought, but looking at it currently, I'm pretty sure PluginContext exists in every usage of ConsoleContext. So maybe that's a historical remnant too, before Rollup made it exist in every hook.

RollupContext also has a hasContext var that it checks against as well, and if falsey, it falls back to the same behavior as ConsoleContext anyway. And I remove that var in #374, because that's never the case anymore. So definitely seems like a historical remnant.

So I believe ConsoleContext is entirely redundant nowadays.

And the main behavior difference is that printing error in PluginContext would kill the build , but printing error in ConsoleContext would not, that's why RollupContext has this bail check to downgrade error to warning

Yea that was a central difference, but at least in index, ConsoleContext is never even used when an error might be displayed. It is passed into some other functions, namely printDiagnostics for tsconfig checks, that could result in an error though. But in those cases, I feel like we probably should bail out anyway -- a tsconfig error or compilerOptions error breaks a bunch of stuff, so erroring out makes sense to me. Let me know if you think otherwise though.

There is also some cosmetic output differences I think.

Yea with warn and error, Rollup adds some formatting + coloring and plugin name + [!] or (!) as a prefix. For warnings, the onwarn callback is also triggered.

But, per above, I don't think those differences are ever meaningful either.

So I'm pretty confident now that ConsoleContext is redundant / no longer necessary.

But if agreed, I'll remove it in a separate PR. For here, I'll just use RollupContext instead

agilgur5 commented 2 years ago

Yea, that's what I thought, but looking at it currently, I'm pretty sure PluginContext exists in every usage of ConsoleContext. So maybe that's a historical remnant too, before Rollup made it exist in every hook.

It is passed into some other functions, namely printDiagnostics for tsconfig checks, that could result in an error though. But in those cases, I feel like we probably should bail out anyway -- a tsconfig error or compilerOptions error breaks a bunch of stuff, so erroring out makes sense to me. Let me know if you think otherwise though.

Ah, nevermind, it looks like options is indeed still the exception to the rule.

~So that means that maybe I shouldn't change hasContext in #374, and instead just remove ConsoleContext, since with the hasContext check, they have identical behavior when no PluginContext~ EDIT: that got merged, so nvm. Since it holds state, it might be simpler / less confusing to have them separate, but may want to merge them into one file at least since their implementations are near identical.

ezolenko commented 2 years ago

I think some other PRs I merged today broke this one

agilgur5 commented 2 years ago

Yea #373 was expected to conflict with this one (as I noted there) due to both using buildEnd, so one or the other was going to need to be updated.

I also still need to make that RollupContext change, then this should be good to go

agilgur5 commented 2 years ago

@ezolenko I think this PR is ready to go now. I fixed the merge conflict, used RollupContext (only for this new check, I didn't change walkTree for now), and also added an integration test for this.

CI is currently failing due to #377, not the code from this PR.

Also happened to stumble upon yet another bug (or two?) in an upstream codebase with this test: https://github.com/facebook/jest/issues/7136 roughly describes it, but basically mocks have poor isolation. Was able to workaround that one with a small refactor. Couldn't workaround another one, that mocks seem to lose state if an error is hit; see my commit message for more details.

Unrelated, but @ezolenko can you give a 👍 / 👎 on the proposal in #228? Have a contributor waiting for approval from you before tackling. (for reference, the proposal is mine, the contributor was going to implement it).

agilgur5 commented 2 years ago

geh, got another Windows-specific test failure on master when this was merged 😕 😐 😐

will debug and try to get a PR out for that, but as usual, I don't work on a Windows machine, so may take a bit of time

EDIT: Fixed in #385

agilgur5 commented 2 years ago

Ah, nevermind, it looks like options is indeed still the exception to the rule.

options is still the exception to the rule, but we don't modify the options at all, so we can actually use buildStart, which was introduced at the same time as buildEnd in Rollup v0.60.

Somehow didn't think of that till now, but that should obviate the need for ConsoleContext and simplify error code so that it all uses context.error instead of some code throwing instead when that wasn't available. This will take a bit of refactoring, but should simplify & standardize a decent bit of code