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.

Sign up for GitHub

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: restore stats output target functionality. #3030

Merged
merged 12 commits into from
Sep 24, 2021
Merged

fix: restore stats output target functionality. #3030

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
aa7fd72
fix: restore stats output target functionality.
splitinfinities Aug 26, 2021
f05890b
Feedback from my own review
splitinfinities Aug 26, 2021
da2e392
prettier
splitinfinities Aug 26, 2021
a58bc3c
Adding stats and feedback from Lars
splitinfinities Sep 9, 2021
d0f7be8
Expand out typing
splitinfinities Sep 9, 2021
907f105
Adding feedback from Ryan, improving typings
splitinfinities Sep 10, 2021
e8e662a
Switch back to parallel builds
splitinfinities Sep 22, 2021
4ff5749
prettier
splitinfinities Sep 22, 2021
d1e73fe
Fix test
splitinfinities Sep 22, 2021
807caa9
Update the components number
splitinfinities Sep 22, 2021
d5bc274
Updating name of new return type
splitinfinities Sep 22, 2021
303af48
Merge branch 'master' into STENCIL-43-fix-stats-flag
rwaskiewicz Sep 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/compiler/output-targets/dist-lazy/lazy-output.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ export const outputLazy = async (config: d.Config, compilerCtx: d.CompilerCtx, b
buildCtx.es5ComponentBundle = result.buildCtx.es5ComponentBundle;
} else if (result.name === 'esm-browser') {
buildCtx.esmBrowserComponentBundle = result.buildCtx.esmBrowserComponentBundle;
buildCtx.buildResults = result.buildCtx.buildResults;
buildCtx.components = result.buildCtx.components;
Comment on lines +79 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we're doing this/what's going on here - can you explain this to me please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! The components and buildResults are passed on to the buildCtx within this file from the resultant builds. These needed to be set on the buildCtx here because the promises don't update the buildCtx any more, and the original stats code needs to understand the state of the updated buildCtx after the promises all run. I wrote it this way so that we could keep the parallelization of the builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this affect the memory usage of Stencil in a negative way? I mean, I'm sure there's some cost associated with it, but do you have an idea of the order of magnitude?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect this comment still holds true, and may likely improve since I brought the parallelization back.

I was worried that adding these to the build context would increase the memory usage significantly. Luckily that is not the case. I ran 10 builds of ionic-framework using master and this PR. The increase is relatively minor:

Build # master #3030
1 MEM: 682.1MB MEM: 658.6MB
2 MEM: 743.8MB MEM: 677.3MB
3 MEM: 546.5MB MEM: 757.6MB
4 MEM: 727.2MB MEM: 697.9MB
5 MEM: 731.8MB MEM: 658.1MB
6 MEM: 730.2MB MEM: 656.2MB
7 MEM: 738.5MB MEM: 749.1MB
8 MEM: 726.2MB MEM: 655.7MB
9 MEM: 709.2MB MEM: 684.9MB
10 MEM: 663.2MB MEM: 617.7MB
Average MEM: 681.3MB MEM: 699.9MB

}
});

Expand Down