-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
refactor(reactivity): reduce size of collectionHandlers #12152
refactor(reactivity): reduce size of collectionHandlers #12152
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/reactivity
@vue/compiler-ssr
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
@vue/compat
vue
commit: |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Flawless PR description! Great job Skirtles! |
What method did you use to accomplish this? |
I have some scripts at https://github.com/skirtles-code/github-analysis-scripts that I used to download the open PRs. Once I had the changes locally I just searched for changes to |
The diff looks horrendous... the change really isn't that complicated.
Bundle size
My primary goal was to reduce the bundle size. I've removed a lot of redundancy from
collectionHandlers.ts
. That involved moving around a lot of code, but it essentially all still works the same way.To make this a bit easier to review I've split it up into individual commits. The first commit is the key change to understand what I've done.
createInstrumentations
was previously creating 4 lots of instrumentations and then returning them. The new version just creates the instrumentations for a single case, based on the argumentsreadonly
andshallow
. Just that one change reduces the bundle size by about 566 bytes, or roughly 135 bytes after gzipping.The other commits gradually inline each of the functions. I checked after each commit to ensure that they really did reduce the bundle size further.
The final result is about 220 bytes saved after gzipping. In the Usages size checks that's a saving of about 1%.
Fixes minor bugs
A key part of the new approach is that
readonly
andshallow
are arguments passed tocreateInstrumentations
, which are then caught in the closure of the specific handlers. Previously these argument were passed directly to the handlers.The old approach could lead to minor bugs in edge cases, due to the ghost parameters:
The reason for the inconsistency in this example is that the
add
method takes_isShallow
as its second argument. This is only supposed to be passed internally, but it also gets passed automatically byforEach
in the example above.With the refactoring in this PR, those ghost parameters are removed, so these kinds of inconsistencies should no longer occur:
Performance
I don't think this PR should have a significant impact on performance. Inlining the handlers may have slightly improved performance in some cases, as it removes an extra function call.
For example, this Playground gives slightly quicker times for me when testing in Chrome:
Impact on other PRs
Such a large amount of churn, albeit within a single file, is a cause for concern. I've checked how this would impact existing PRs.
These PRs would conflict with this PR:
reactive set
supports the difference, intersection, isDisjointFrom, isSubsetOf, isSupersetOf, and symmetricDifference methods #11399 - The conflicts should be easily fixable. If anything, the changes here will simplify that PR, as it currently repeats much of the code.These PRs make changes to
collectionHandler.ts
, but I don't think this PR would cause any conflicts: