Skip to content

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
@agilgur5 agilgur5 force-pushed the only-emit-types-once branch from 3c06698 to 9a9c588 Compare April 20, 2020 04:28
- 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 😮

@agilgur5 agilgur5 force-pushed the only-emit-types-once branch from bfae7d9 to 714e4bd Compare April 20, 2020 04:42
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