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

agilgur5 · 2022-06-05T01:33:23Z

Summary

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

Fixes type-checking doesn't work on type-only modules #298 (which uses include globs)
Fixes the latter half of Vague failed to transpile error -- noEmitOnError: true breaks rpt2 #254 (which uses include globs)
Partial workaround for Type only imports are not set as dependencies #7, but not complete (see details below)
Similarly partially fixes some things mentioned in Declarations not generated for type-only files not explicitly specified in tsconfig #211, but does not truly/completely fix it as it uses files (see details below)
- See my root cause analysis in type-checking doesn't work on type-only modules #298 (comment) for how I plan to fix that
Partially fixes the most common issue that I mentioned in fix(github): improve formatting of issue template spoilers #311 (comment), type-only imports

Details

type-only files never get processed by Rollup as their imports are elided/removed by TS in the resulting compiled JS file
- so, as a workaround, make sure that all files in the tsconfig include (i.e. in parsedConfig.fileNames) are also type-checked
  - note that this will not catch all type-only imports, in particular if one is using tsconfig files as in Declarations not generated for type-only files not explicitly specified in tsconfig #211 (or in general not using glob patterns in include) -- this is just a workaround, that requires a separate fix
- we do the same process for generating declarations for "missed" files right now in _onwrite, so basically do the same thing but for type-checking in ~~_ongenerate~~ EDIT: moved to buildEnd (see below)

(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 #298 (comment)

Review Notes

~~This is built on top of refactor: split out a common typecheckFile func #344, please merge that first. Marked this as a draft for now~~
- EDIT: rebased on top now that it was merged
~~I'm using the declarations dict as a way to detect if a file has already run through the transform so as not to duplicate type-checking (both for performance and to not duplicate error messages)~~. ~~I'm not sure if this is optimal though, I merely copied _onwrite's missed declaration check~~.
- EDIT: nvm, that wouldn't work given that you can type-check without outputting any declarations, created a new Set to track this instead
- ~~This also runs for each Rollup output, so de-duping is pretty important in that sense too~~
  - EDIT: moved to the buildEnd hook instead, so it doesn't run for each output anymore (and it doesn't generate any output files, unlike the missed declarations, so buildEnd is a more correct place for it as well)
This exits successfully with zero on a type-error, while tsc will exit non-zero and not emit by default. emitSkipped is false too during the declaration output, despite the type-check error.
- I could exit out with this.error in this case as well, but would have to be a different error message

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 · 2022-06-28T22:46:08Z

@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 · 2022-06-29T06:21:06Z

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 · 2022-06-29T22:07:30Z

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 · 2022-06-29T22:47:08Z

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 · 2022-06-29T23:03:03Z

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 · 2022-07-01T04:19:52Z

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 · 2022-07-02T02:29:23Z

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 · 2022-07-05T05:22:54Z

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 · 2022-07-05T16:24:27Z

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 · 2022-07-05T16:33:39Z

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 · 2022-07-06T04:15:08Z

I think some other PRs I merged today broke this one

agilgur5 · 2022-07-06T21:10:31Z

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

- type-only files never get processed by Rollup as their imports are elided/removed by TS in the resulting compiled JS file - so, as a workaround, make sure that all files in the `tsconfig` `include` (i.e. in `parsedConfig.fileNames`) are also type-checked - note that this will not catch _all_ type-only imports, in particular if one is using `tsconfig` `files` (or in general _not_ using glob patterns in `include`) -- this is just a workaround, that requires a separate fix - we do the same process for generating declarations for "missed" files right now in `_onwrite`, so basically do the same thing but for type-checking in `_ongenerate` (_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))

- `buildEnd` is a more correct place for it, since this does not generate any output files - (unlike the missed declarations) - and this means it's only called once per build, vs. once per output - remove the check against the `declarations` dict as one can type-check without outputting declarations - i.e. if `declaration: false`; not the most common use-case for rpt2, but it is one

- since `parsedConfig.fileNames` could include files that were already checked during the `transform` hook - and because `declarations` dict is empty when `declaration: false`, so can't check against that

- because printing diagnostics can bail if the category was error - that can result in a file being type-checked but not added to checkedFiles

…rateBundle - per ezolenko, the whole generateRound etc stuff was a workaround because the buildEnd hook actually _didn't exist_ before - so now that it does, we can use it to simplify some logic - no longer need `_ongenerate` as that should be in `buildEnd`, and no longer need `_onwrite` as it is the only thing called in `generateBundle`, so just combine them - importantly, `buildEnd` also occurs before output generation, so this ensures that type-checking still occurs even if `bundle.generate()` is not called - also move the `walkTree` call to above the "missed" type-checking as it needs to come first - it does unconditional type-checking once per watch cycle, whereas "missed" only type-checks those that were, well, "missed" - so in order to not have duplication, make "missed" come after, when the `checkedFiles` Set has been filled by `walkTree` already - and for simplification, just return early on error to match the current behavior - in the future, may want to print the error and continue type-checking other files - so that all type-check errors are reported, not just the first one NOTE: this is WIP because the `cache.done()` call and the `!noErrors` message are incorrectly blocked behind the `pluginOptions.check` conditional right now - `cache.done()` needs to be called regardless of check or error or not, i.e. in all scenarios - but ideally it should be called after all the type-checking here - `!noErrors` should be logged regardless of check or not - and similarly, after the type-checking

- instead of making a big `if` statement, decided to split out a `buildDone` function - to always call at the end of the input phase - we can also move the `cache().done()` in `emitSkipped` into `buildEnd`, as `buildEnd` gets called when an error occurs as well - and this way we properly print for errors as well - `buildDone` will have more usage in other PRs as well, so I figure it makes sense to split it out now as well

- i.e. bail out when `abortOnError: true`, which `ConsoleContext` can't do - `ConsoleContext` is basically meant for everywhere `RollupContext` can't be used - which is effectively only in the `options` hook, per the Rollup docs: https://rollupjs.org/guide/en/#options

- now that the integration tests exist, we can actually test this scenario - refactor: give each test their own `onwarn` mock when necessary - while `restoreMocks` is set in the `jest.config.js`, Jest apparently has poor isolation of mocks: jestjs/jest#7136 - if two tests ran in parallel, `onwarn` was getting results from both, screwing up the `toBeCalledTimes` number - couldn't get the syntax error to work with `toBeCalledTimes` either - if no mock is given, it _does_ print warnings, but if a mock is given, it doesn't print, yet isn't called? - I think the mock is getting screwed up by the error being thrown here; maybe improperly saved or something

agilgur5 · 2022-07-07T01:26:51Z

@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: jestjs/jest#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 · 2022-07-12T18:07:38Z

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 · 2022-07-30T19:02:24Z

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

agilgur5 added kind: bug Something isn't working properly topic: type-only / emit-less imports Related to importing type-only files that will not be emitted labels Jun 5, 2022

agilgur5 linked an issue Jun 5, 2022 that may be closed by this pull request

Vague failed to transpile error -- noEmitOnError: true breaks rpt2 #254

Closed

agilgur5 mentioned this pull request Jun 14, 2022

refactor: split out a common typecheckFile func #344

Merged

agilgur5 force-pushed the fix-partial-type-only-typecheck branch from aeb11a7 to be34970 Compare June 28, 2022 21:41

agilgur5 marked this pull request as ready for review June 28, 2022 22:35

agilgur5 mentioned this pull request Jun 29, 2022

test: add initial integration test suite #371

Merged

agilgur5 mentioned this pull request Jun 30, 2022

fix(diagnostics): pretty defaults to true in TS 2.9+ #372

Merged

This was referenced Jul 2, 2022

fix(diagnostics): workaround Rollup duplicating error messages #373

Merged

clean: remove backward-compat checks for old Rollup versions #374

Merged

agilgur5 added 4 commits July 6, 2022 17:37

add new checkedFiles Set to not duplicate type-checking

5e6fc1e

- since `parsedConfig.fileNames` could include files that were already checked during the `transform` hook - and because `declarations` dict is empty when `declaration: false`, so can't check against that

move checkedFiles.add to the beginning of typecheckFile

9ba4ced

- because printing diagnostics can bail if the category was error - that can result in a file being type-checked but not added to checkedFiles

agilgur5 added 3 commits July 6, 2022 17:42

agilgur5 force-pushed the fix-partial-type-only-typecheck branch from 49b26f4 to 3a027ca Compare July 6, 2022 22:00

ezolenko merged commit eab48a3 into ezolenko:master Jul 12, 2022

agilgur5 mentioned this pull request Jul 12, 2022

jest.clearAllMocks(); does not remove mock implementation within afterEach jestjs/jest#7136

Open

agilgur5 added the topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX label Jul 12, 2022

agilgur5 mentioned this pull request Jul 20, 2022

fix: don't duplicate type-only check on Windows #385

Merged

agilgur5 added the scope: tests Tests could be improved. Or changes that only affect tests label Jul 20, 2022

This was referenced Jul 22, 2022

test: add initial watch mode test suite #386

Merged

refactor: move generateRound = 0 to buildEnd #390

Merged

agilgur5 mentioned this pull request Jul 29, 2022

fix: handle all type-only imports by piping TS imports #406

Merged

agilgur5 mentioned this pull request Jul 30, 2022

refactor: combine two context files into one #396

Merged

agilgur5 mentioned this pull request Aug 25, 2022

clean: remove ConsoleContext entirely by using buildStart #414

Merged

agilgur5 mentioned this pull request Sep 17, 2022

Type-only/Emit-less TS files aren't being checked jaredpalmer/tsdx#725

Open

agilgur5 deleted the fix-partial-type-only-typecheck branch July 2, 2023 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

agilgur5 commented Jun 5, 2022 •

edited

agilgur5 commented Jun 28, 2022

ezolenko commented Jun 29, 2022

agilgur5 commented Jun 29, 2022 •

edited

agilgur5 commented Jun 29, 2022

agilgur5 commented Jun 29, 2022 •

edited

agilgur5 commented Jul 1, 2022 •

edited

agilgur5 commented Jul 2, 2022 •

edited

ezolenko commented Jul 5, 2022 •

edited by agilgur5

agilgur5 commented Jul 5, 2022

agilgur5 commented Jul 5, 2022 •

edited

ezolenko commented Jul 6, 2022

agilgur5 commented Jul 6, 2022

agilgur5 commented Jul 7, 2022

agilgur5 commented Jul 12, 2022 •

edited

agilgur5 commented Jul 30, 2022 •

edited

fix: type-check included files missed by transform (type-only files) #345

fix: type-check included files missed by transform (type-only files) #345

Conversation

agilgur5 commented Jun 5, 2022 • edited

Summary

Details

Review Notes

Misc

agilgur5 commented Jun 28, 2022

ezolenko commented Jun 29, 2022

agilgur5 commented Jun 29, 2022 • edited

agilgur5 commented Jun 29, 2022

agilgur5 commented Jun 29, 2022 • edited

agilgur5 commented Jul 1, 2022 • edited

agilgur5 commented Jul 2, 2022 • edited

ezolenko commented Jul 5, 2022 • edited by agilgur5

agilgur5 commented Jul 5, 2022

agilgur5 commented Jul 5, 2022 • edited

ezolenko commented Jul 6, 2022

agilgur5 commented Jul 6, 2022

agilgur5 commented Jul 7, 2022

agilgur5 commented Jul 12, 2022 • edited

agilgur5 commented Jul 30, 2022 • edited

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

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

agilgur5 commented Jun 5, 2022 •

edited

agilgur5 commented Jun 29, 2022 •

edited

agilgur5 commented Jun 29, 2022 •

edited

agilgur5 commented Jul 1, 2022 •

edited

agilgur5 commented Jul 2, 2022 •

edited

ezolenko commented Jul 5, 2022 •

edited by agilgur5

agilgur5 commented Jul 5, 2022 •

edited

agilgur5 commented Jul 12, 2022 •

edited

agilgur5 commented Jul 30, 2022 •

edited