-
-
Notifications
You must be signed in to change notification settings - Fork 850
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
feature: optimize Immer bundling #536
Conversation
Import size report for immer: ┌───────────────────────┬───────────┬────────────┬───────────┐ │ (index) │ just this │ cumulative │ increment │ ├───────────────────────┼───────────┼────────────┼───────────┤ │ import * from 'immer' │ 6761 │ 0 │ 0 │ │ produce │ 4048 │ 4048 │ 0 │ │ enableES5 │ 4908 │ 4916 │ 868 │ │ enableMapSet │ 5007 │ 5769 │ 853 │ │ enablePatches │ 4836 │ 6544 │ 775 │ └───────────────────────┴───────────┴────────────┴───────────┘ (this report was generated by npmjs.com/package/import-size)
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d676e61:
|
setAutoFreeze, | ||
setUseProxies, | ||
enableAllPlugins | ||
} from "../dist/immer.cjs.production.min.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see some benefit to refer minified prod build in tests? For once it might be harder to run debugger with it. You could simply refer to ../dist
here which will pickup dev build so it would be less verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, these are the performance test, not the unit tests. So they won't really assert anything and arent to be relied upon for correctness. Using a prod build to better reflect raw production performance
@@ -0,0 +1,17 @@ | |||
module.exports = { | |||
moduleNameMapper: { | |||
"src/.*": "<rootDir>/dist/immer.esm.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Shouldn't you refer to src
in test files and let it translated to dist with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this makes the tests behave as if immer was a dependency. So if anything is referred from src that is also exported from the index, it is fine, otherwise it will become undefined, so any graybox tests are marked to be not included in this build config
package.json
Outdated
@@ -1,27 +1,30 @@ | |||
{ | |||
"name": "immer", | |||
"version": "5.3.2", | |||
"version": "6.0.0-alpha.2", | |||
"description": "Create your next immutable state by mutating the current one", | |||
"main": "dist/immer.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"main": "dist/immer.js", | |
"main": "dist/index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 😅
@mweststrate Have you noticed my review comments above? :) |
yes, will look into it once done :) this is still a draft, just coudn't
find that draft button anymore :P
…On Fri, Feb 21, 2020 at 7:57 PM Daniel K. ***@***.***> wrote:
@mweststrate <https://github.com/mweststrate> Have you noticed my review
comments above? :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#536?email_source=notifications&email_token=AAN4NBEUESXAKFBKW76P4WLREAWZ5A5CNFSM4KWZNJB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMT44BI#issuecomment-589811205>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBCR6HIXFVXY6JPIUDLREAWZ5ANCNFSM4KWZNJBQ>
.
|
@markerikson (and others) the new version can be tried as [email protected]. Installation instructions can be found here: https://github.com/immerjs/immer/blob/multi-bundle/docs/installation.md#pick-your-immer-version |
@mweststrate You should definitely fix that |
Can confirm the entry point issue broke things. I just tried @mweststrate , can you put out another alpha? (Given the time zone difference, you're probably asleep now, so I'll have to try this again tomorrow night.) Having said that, I also tried a local test build of RTK with the hand-edited copy of Immer 6, and I can confirm that the size of Immer in a demo app bundle is smaller. Webpack reports the demo app's
So, looks like that plugin adds about 2K in this example, but overall, a definite win. |
I'm actually in CA at the moment :) Just pushed an update (didn't test it,
let me know if it isn't ok)
Yeah those numbers align with my expectations for non-gzipped code. I think
the ES5 plugin could actually be pruned a little bit more as there seems to
be some almost-duplication, maybe in a next version.
…On Tue, Feb 25, 2020 at 7:18 PM Mark Erikson ***@***.***> wrote:
Can confirm the entry point issue broke things. I just tried alpha.3, and
yeah, all our tests blew up with Jest saying it can't find Immer.
Hand-editing node_modules/immer/package.json let them run okay.
@mweststrate <https://github.com/mweststrate> , can you put out another
alpha? (Given the time zone difference, you're probably asleep now, so I'll
have to try this again tomorrow night.)
Having said that, I also tried a local test build of RTK with the
hand-edited copy of Immer 6, and I can confirm that the size of Immer in a
demo app bundle is smaller.
Webpack reports the demo app's main.js went from 19.1K down to 11.5K for
Immer 6 with no plugins, and 13.5K with the ES5 plugin.
source-map-explorer was previously showing Immer as being around 12K by
itself, I think. It now shows 6.64K with no plugins and 8.61K with the ES5
plugin.
So, looks like that plugin adds about 2K in this example, but overall, a
definite win.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#536?email_source=notifications&email_token=AAN4NBAYCX2FRXCHZXJJZ6LREXNSBA5CNFSM4KWZNJB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM6UI6I#issuecomment-591217785>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBFZZEIAF2QARWWLFOLREXNSBANCNFSM4KWZNJBQ>
.
|
Yep, |
add 5.0.2 type definitions as fallback for TS<3.7
🎉 This PR is included in version 6.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary: ## The dependency [immer](https://github.com/immerjs/immer) was updated from `5.3.6` to `6.0.0`. This version is **not covered** by your **current version range**. If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update. --- **Publisher:** [aleclarson](https://www.npmjs.com/~aleclarson) **License:** MIT <details> <summary>Release Notes for v6.0.0</summary> <h1><a href="https://urls.greenkeeper.io/immerjs/immer/compare/v5.3.6...v6.0.0">6.0.0</a> (2020-03-03)</h1> <ul> <li>Merge pull request <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="566542863" data-permission-text="Title is private" data-url="immerjs/immer#536" data-hovercard-type="pull_request" data-hovercard-url="/immerjs/immer/pull/536/hovercard" href="https://urls.greenkeeper.io/immerjs/immer/pull/536">https://github.com/facebook/flipper/issues/536</a> from immerjs/multi-bundle (<a href="https://urls.greenkeeper.io/immerjs/immer/commit/0d87fc88e8efffdacbb5db295cb9efd624cd2757">0d87fc8</a>), closes <a href="https://urls.greenkeeper.io/immerjs/immer/issues/536" data-hovercard-type="pull_request" data-hovercard-url="/immerjs/immer/pull/536/hovercard">https://github.com/facebook/flipper/issues/536</a></li> </ul> <h3>BREAKING CHANGES</h3> <ul> <li>Support for ES5, patches and Map/Set collections has to be <em>explicitly</em> enable now: <a href="https://immerjs.github.io/immer/docs/installation" rel="nofollow">https://immerjs.github.io/immer/docs/installation</a></li> <li>Custom serialization hooks are no longer supported</li> </ul> <p>feat: Adding large data sets to a draft has been optimized (in case autofreeze is disabled)</p> </details> <details> <summary>Commits</summary> <p>The new version differs by 41 commits.</p> <ul> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/0d87fc88e8efffdacbb5db295cb9efd624cd2757"><code>0d87fc8</code></a> <code>Merge pull request #536 from immerjs/multi-bundle</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/d676e61b638a0611bc105e62523c2c6fc7f97531"><code>d676e61</code></a> <code>removed notes file</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/3cfd62103b144057289c0eff9269bedd09265895"><code>3cfd621</code></a> <code>backported new API's</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/0d7f883067dc30320a52e9df4e324abfec12f8dc"><code>0d7f883</code></a> <code>Merge pull request #541 from phryneas/typesversions-compat</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/f215409d2ea6249fea1bab907f87c7a25a231f68"><code>f215409</code></a> <code>Merge branch 'multi-bundle' into typesversions-compat</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/5f86272a3a8fe6c6f3edd0e48a4edd4e912b3748"><code>5f86272</code></a> <code>Fixed entry point</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/b94c34fc81f2de4b40c181fa221e2daef16ff34d"><code>b94c34f</code></a> <code>Merge pull request #544 from delanni/patch-1</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/30e0f54d306ea28c895d8e34d9a30a372a43ffc3"><code>30e0f54</code></a> <code>Add section about using classes with immer</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/e6c3f13479fe2407b4d06decbb8c2440e4e92811"><code>e6c3f13</code></a> <code>stricter compression</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/7a38af91d2e9b1fe359c1671942057ebff9be824"><code>7a38af9</code></a> <code>unclassed scope</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/cb3b9d68397e111272cbb3039717255b278f6bb9"><code>cb3b9d6</code></a> <code>Processed some todo's</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/895e0cc9f885cb88086cf6d1515e33e9993cad25"><code>895e0cc</code></a> <code>Updated docs</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/edb1cad865c405bb34823afb056cfc6d73fc32de"><code>edb1cad</code></a> <code>Renamed utils dir</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/db1652018ac6f40dea9e090a4d589cc072d7e456"><code>db16520</code></a> <code>Tests for plugins</code></li> <li><a href="https://urls.greenkeeper.io/immerjs/immer/commit/ab49edc11cd40dc8e441e42e67e92596bf9abecf"><code>ab49edc</code></a> <code>Moving files into folders</code></li> </ul> <p>There are 41 commits in total.</p> <p>See the <a href="https://urls.greenkeeper.io/immerjs/immer/compare/21c1597f1721f0ff34f83121c87ad8ee19227c55...0d87fc88e8efffdacbb5db295cb9efd624cd2757">full diff</a></p> </details> --- <details> <summary>FAQ and help</summary> There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new). </details> --- Your [Greenkeeper](https://greenkeeper.io) bot 🌴 Pull Request resolved: #853 Reviewed By: passy Differential Revision: D20303311 Pulled By: mweststrate fbshipit-source-id: ba6ab68246144c858490fbffca8bcff641b930ad
BREAKING CHANGE: need to enable features
BREAKING CHANGE: custom hooks are no longer supporterd