-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[SVELTE] rollup -private for winnings #4925
Conversation
Yay @runspired! I'm excited for this change! |
@igorT @bmac @stefanpenner our pre-gzip size has shifted significantly, but our post-gzip size has not budged much. I suspect some of this is due to us still needing to re-export almost everything from https://github.com/emberjs/data/pull/4925/files#diff-d090d6077afd998d4578d3423aa53f1a |
Can you share what the costs are to build time for both standard development builds and production builds before and after this change? |
LGTM (in broad strokes) Be sure to also measure:
|
@stefanpenner gonna port those TODOs up to the main PR text |
|
@stefanpenner given there is a new branch on my fork with a slightly different name and the fix you mention here, I suspect you trolled yourself. I think we want to remove the "merge into master" commit, do an actual rebase, and apply this commit? runspired@a560ada |
I think i didn't finish the rebase, redoing it now. |
4622a04
to
7291441
Compare
dev/prod builds feel fine now, quick load profile doesn't show any obvious issues. |
Thanks @stefanpenner! I'll get a HAR on this asap. |
@runspired ping |
@stefanpenner I did, I'll share data later, it's basically "slight improvement" |
@runspired awesome. So we are good to go? (i'll fix the rebase now) |
7291441
to
77bce45
Compare
77bce45
to
f15c67b
Compare
Rebased. @runspired let me know if you are good with me merging: initial build added cost: 400ms Not perfect, but not the end of the world. Some future work to allow for "prebuilt" assets will mitigate that (and more importantly the 5s of babel that is pre-existing) |
@stefanpenner no objections to merging. |
🎉 |
This PR de-cycles our import/export structure into and out-of
-private
, allowing us to use rollup to condense the-private
directory.It took 2 liberties with file structure changes to differentiate between public and private:
addon/instance-initializers
setup-container
(and could be made a bit nicer still)Additionally,
Transform
and AdapterErrors
are now re-exported from-private
, as they are occasionally used in the internals. I don't think transforms and errors belong there and only a very tiny portion of them is actually used internally. As they are entirely public interface we should consider how to divide / extract.TODO
ensure globals build gets the same winningsBefore
After