Combine rollup configs with multiconfig and add size snapshot #405
Conversation
TrySound
commented
Mar 24, 2018
•
TrySound
commented
- combined both umd/min configs with rollup multiconfig
- changed umd bundle to development env like react does
- added my size-snapshot plugin to simplify size tracking and improving in some cases. Later we can probably bundle also cjs and esm to track their treeshakability.
|
Brilliant! I will take a closer look on monday |
|
I am trying to think of what the development flow would be with this new plugin. Can you please elaborate on this? Also, how would we go about rebase lining? |
|
Every new commit will show the difference of size. Some commits may add too heavy dependency, some can be better minified. Also I'm gonna propose in the next PR esm and cjs bundles for entry points instead of babeled files. For esm bundle size-snapshot is able to show treeshakability. To protect from commits without snapshot update we may add environment for ci like sizeSnapshot({
updateSnapshot: process.env.SNAPSHOT !== 'check'
})and size snapshot will fail if existing snapshot is not matched with computed one. |
|
Okay, so you would need to run the command to generate the size snapshot. Would it be good to have a seperate test script which simply checks to see if the bundle size has grown? That way we could run it as a build check |
|
@alexreardon Done. |
package.json
Outdated
| @@ -25,15 +25,15 @@ | |||
| "/src" | |||
| ], | |||
| "scripts": { | |||
| "test": "jest --config ./jest.config.js", | |||
| "test": "cross-env SNAPSHOT=check yarn build:dist && jest --config ./jest.config.js", | |||
There was a problem hiding this comment.
is there any way we can run the test without generating the dist itself? hmm...
There was a problem hiding this comment.
AFAIK there's no way to prevent rollup writing currently. Is this a problem?
There was a problem hiding this comment.
The idea of the plugin was integration in bundler without additional configuration.
There was a problem hiding this comment.
Can this create a scenario where a merge to master can break? Thinking out loud.. i suppose not if it it updating the snapshot too
There was a problem hiding this comment.
Can we run the check as the second step of the command? (After jest)
There was a problem hiding this comment.
@alexreardon It can if PR is not based on actual master. But I think it's not so critical.
|
👌
…On Thu, 29 Mar 2018 at 6:11 pm, Bogdan Chadkin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In package.json
<#405 (comment)>
:
> @@ -25,15 +25,15 @@
"/src"
],
"scripts": {
- "test": "jest --config ./jest.config.js",
+ "test": "cross-env SNAPSHOT=check yarn build:dist && jest --config ./jest.config.js",
The idea of the plugin was integration in bundler without additional
configuration.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#405 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACFN7YPFRwvTTmo2talulUxRMAM51fbdks5tjIkDgaJpZM4S5sqC>
.
|
|
@alexreardon Can we consider using bundles for cjs and esm before major 7 release. I'd like to open PR after this one get merged. |
package.json
Outdated
| @@ -25,15 +25,15 @@ | |||
| "/src" | |||
| ], | |||
| "scripts": { | |||
| "test": "jest --config ./jest.config.js", | |||
| "test": "cross-env SNAPSHOT=check yarn build:dist && jest --config ./jest.config.js", | |||
There was a problem hiding this comment.
Can this create a scenario where a merge to master can break? Thinking out loud.. i suppose not if it it updating the snapshot too
package.json
Outdated
| @@ -25,15 +25,15 @@ | |||
| "/src" | |||
| ], | |||
| "scripts": { | |||
| "test": "jest --config ./jest.config.js", | |||
| "test": "cross-env SNAPSHOT=check yarn build:dist && jest --config ./jest.config.js", | |||
There was a problem hiding this comment.
Can we run the check as the second step of the command? (After jest)
|
Done |
|
Well done @TrySound! We'll see how the snapshot watching goes. If it causes too much friction we might remove it at some point. However, for now I am keen to have more visibility around our bundle size changes |
