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

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

Merged
merged 8 commits into from
Jul 12, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jun 5, 2022

Summary

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

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
    • 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
Copy link
Collaborator Author

@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
Copy link
Owner

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
Copy link
Collaborator Author

agilgur5 commented Jun 29, 2022

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

agilgur5 commented Jun 29, 2022

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
Copy link
Collaborator Author

agilgur5 commented Jul 1, 2022

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
Copy link
Collaborator Author

agilgur5 commented Jul 2, 2022

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
Copy link
Owner

ezolenko commented Jul 5, 2022

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
Copy link
Collaborator Author

agilgur5 commented Jul 5, 2022

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
Copy link
Collaborator Author

agilgur5 commented Jul 5, 2022

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
Copy link
Owner

ezolenko commented Jul 6, 2022

I think some other PRs I merged today broke this one

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 6, 2022

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
@agilgur5 agilgur5 force-pushed the fix-partial-type-only-typecheck branch from 49b26f4 to 3a027ca Compare July 6, 2022 22:00
- 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
Copy link
Collaborator Author

agilgur5 commented Jul 7, 2022

@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
Copy link
Collaborator Author

agilgur5 commented Jul 12, 2022

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 agilgur5 added the topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX label Jul 12, 2022
@agilgur5 agilgur5 added the scope: tests Tests could be improved. Or changes that only affect tests label Jul 20, 2022
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 30, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly scope: tests Tests could be improved. Or changes that only affect tests topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX topic: type-only / emit-less imports Related to importing type-only files that will not be emitted
Projects
None yet
2 participants