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

Commits on Jul 6, 2022

  1. fix: type-check included files missed by transform (type-only files)

    - 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))
    agilgur5 committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    2716344 View commit details
    Browse the repository at this point in the history
  2. move misssed type-checking to buildEnd hook, remove declarations check

    - `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
    agilgur5 committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    53604ff View commit details
    Browse the repository at this point in the history
  3. add new checkedFiles Set to not duplicate type-checking

    - 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
    agilgur5 committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    5e6fc1e View commit details
    Browse the repository at this point in the history
  4. move checkedFiles.add to the beginning of typecheckFile

    - 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 committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    9ba4ced View commit details
    Browse the repository at this point in the history
  5. wip: fuse _ongenerate functionality into buildEnd, _onwrite into gene…

    …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
    agilgur5 committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    0b98ab3 View commit details
    Browse the repository at this point in the history
  6. call cache().done() and !noErrors in check and non-check conditions

    - 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
    agilgur5 committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    c5bca6c View commit details
    Browse the repository at this point in the history
  7. use RollupContext for type-only files

    - 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 committed Jul 6, 2022
    Configuration menu
    Copy the full SHA
    3a027ca View commit details
    Browse the repository at this point in the history

Commits on Jul 7, 2022

  1. add test for type-only file with type errors

    - 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 committed Jul 7, 2022
    Configuration menu
    Copy the full SHA
    01373c1 View commit details
    Browse the repository at this point in the history