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

(optim/fix): only emit type declarations once #691

Merged
merged 2 commits into from
Apr 20, 2020

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Apr 20, 2020

  • every other emission is just a duplicate -- no need to spend compute
    time to make duplicates

    • even for the upcoming multi-entry, the same types are emitted for
      each entry as the compiler just emits types for everything in the
      tsconfig include
  • fixes a long-standing bug with the deprecated moveTypes() function
    that would occasionally cause types to not be properly output

    • this was actually due to moveTypes() being run multiple times in
      parallel (as well as the types themselves being emitted multiple
      times in parallel)
      • hence the EEXIST and ENOENT filesystem errors being thrown, as
        that directory was being changed multiple times in parallel
        • race conditions, fun!
    • now they're only emitted once and only moved once, so no problems!
  • also fixes a bug with an initial version of multi-entry where if an
    entry in a subdir of src/ were added, e.g. src/foo/bar, the entire
    tree of type declarations would get output into dist/foo/src/*.d.ts

    • alternatively, could call moveTypes() with an arg for each entry,
      but there's no need since all declarations get produced the first
      time around anyway
    • similar bug occurred with an initial version of --preserveModules
      that used separate format directories

Also threw in another TS optimization here with the cacheRoot change that's along the same lines but slightly different of a change.


This is extracted out of #367 's 14b5397, but is a better version in a few ways:

  1. Doesn't have issues with declarationMaps as they've also been set to false. Otherwise would get errors / test failures that you can't have declarationMaps without declarations
    • Better testing around this nowadays revealed this bug
  2. : undefined still overrode some declaration/declarationMap settings, also causing errors / test failures, so changed this to not set either on the first emission
  3. moveTypes() was moved into a then of the asyncro() so that it's still part of the progress estimation.

The bugs this fixed in #367 and by extension #535 should no longer be present anyway since #367 switched/improved to use Rollup's code-splitting instead of a separate build & bundle per entry and #535 will need to be updated to use the same output.entryFileNames as a later version of #367 used instead of using separate format dirs.


I also realized that this might fix the bugs with moveTypes() that gave us so many CI issues prior to #504 very recently in #500 (comment).
And indeed this does fix those bugs, hence the removals of the error throwaways and a change in the deprecation message by a bit (it's still a problematic option, with another caveat other than those listed that moveTypes() doesn't apply to custom declarationDirs, though that could be fixed). I tested that this was the case by changing rootDir to ./ in every test fixture and running tests a couple of times and no errors were thrown anymore!


The cacheRoot optimization is also related to #328 / #329. And might make #618 obsolete.

- every other emission is just a duplicate -- no need to spend compute
  time to make duplicates
  - even for the upcoming multi-entry, the same types are emitted for
    each entry as the compiler just emits types for everything in the
    `tsconfig` `include`

- fixes a long-standing bug with the deprecated moveTypes() function
  that would occassionally cause types to not be properly output
  - this was actually due to moveTypes() being run multiple times in
    parallel (as well as the types themselves being emitted multiple
    times in parallel)
    - hence the EEXIST and ENOENT filesystem errors being thrown, as
      that directory was being changed multiple times in parallel
      - race conditions, fun!
  - now they're only emitted once and only moved once, so no problems!

- also fixes a bug with an initial version of multi-entry where if an
  entry in a subdir of src/ were added, e.g. src/foo/bar, the entire
  tree of type declarations would get output into dist/foo/src/*.d.ts
  - alternatively, could call `moveTypes()` with an arg for each entry,
    but there's no need since all declarations get produced the first
    time around anyway
  - similar bug occurred with an initial version of `--preserveModules`
    that used separate format directories
- similarly to the previous commit, it's unnecessary and less
  performant to have a separate cacheRoot per format
  - each TS compilation occurs before the format is changed anyway, so
    at that point in the process, format is irrelevant
  - builds can now re-use cache from other formats, which resulted in
    a perf boost during testing
@agilgur5
Copy link
Collaborator Author

Ah, also should note that from my local test runs, the time it took to run all tests decreased by 10-15%, which is a substantial perf improvement!
If this tiny change was so substantial, switching to per-ouput plugins is really going to be big when building multiple output formats 😮

Copy link
Collaborator Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. straightforward, small diff, and already went through a few iterations.

@agilgur5 agilgur5 merged commit 3357cbf into jaredpalmer:master Apr 20, 2020
@agilgur5 agilgur5 added the kind: optimization Performance, space, size, etc improvement label Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: optimization Performance, space, size, etc improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant