(optim/fix): only emit type declarations once#691
Merged
agilgur5 merged 2 commits intojaredpalmer:masterfrom Apr 20, 2020
Merged
(optim/fix): only emit type declarations once#691agilgur5 merged 2 commits intojaredpalmer:masterfrom
agilgur5 merged 2 commits intojaredpalmer:masterfrom
Conversation
This was referenced 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 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
3c06698 to
9a9c588
Compare
- 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
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! |
bfae7d9 to
714e4bd
Compare
agilgur5
commented
Apr 20, 2020
Collaborator
Author
agilgur5
left a comment
There was a problem hiding this comment.
LGTM. straightforward, small diff, and already went through a few iterations.
This was referenced Oct 6, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
every other emission is just a duplicate -- no need to spend compute
time to make duplicates
each entry as the compiler just emits types for everything in the
tsconfigincludefixes a long-standing bug with the deprecated moveTypes() function
that would occasionally cause types to not be properly output
parallel (as well as the types themselves being emitted multiple
times in parallel)
that directory was being changed multiple times in parallel
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
moveTypes()with an arg for each entry,but there's no need since all declarations get produced the first
time around anyway
--preserveModulesthat used separate format directories
Also threw in another TS optimization here with the
cacheRootchange 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:
declarationMaps as they've also been set tofalse. Otherwise would get errors / test failures that you can't havedeclarationMaps withoutdeclarations: undefinedstill overrode somedeclaration/declarationMapsettings, also causing errors / test failures, so changed this to not set either on the first emissionmoveTypes()was moved into athenof theasyncro()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.entryFileNamesas 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 customdeclarationDirs, though that could be fixed). I tested that this was the case by changingrootDirto./in every test fixture and running tests a couple of times and no errors were thrown anymore!The
cacheRootoptimization is also related to #328 / #329. And might make #618 obsolete.