perf: reduce the bundle size, vol. 2 #24969
Conversation
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5422397678. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5422397678.
|
|
|
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5438070567. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5438070567.
|
(Note: this also depends on a similar design-system PR for DisplayImageUpload.)
iamakulov
force-pushed
the
perf/bundle-size-v2
branch
from
f0dfa31
to
081c778
Compare
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5439060915. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5439060915.
To know the list of identified flaky tests - Refer here |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5439060915. |
| @@ -1,5 +1,5 @@ | |||
| import { BaseQueryGenerator } from "../BaseQueryGenerator"; | |||
| import { format } from "sql-formatter"; | |||
| import { formatDialect, postgresql } from "sql-formatter"; | |||
There was a problem hiding this comment.
@iamakulov , why is this change required?
There was a problem hiding this comment.
That’s because sql-formatter bundles a whole lot of dialect formatters, by default – and we only need one of them: sql-formatter-org/sql-formatter#452
There was a problem hiding this comment.
@iamakulov , shouldn't we also change the format import in other places like
https://github.com/appsmithorg/appsmith/blob/release/app/client/src/WidgetQueryGenerators/MySQL/index.ts#L3
https://github.com/appsmithorg/appsmith/blob/release/app/client/src/WidgetQueryGenerators/Snowflake/index.ts#L2
https://github.com/appsmithorg/appsmith/blob/release/app/client/src/WidgetQueryGenerators/MSSQL/index.ts#L2
There was a problem hiding this comment.
Yes! Looks like these were added recently during a recent merge. Thanks for the catch – I’ll update them and add a ESLint rule as well.
| @@ -1,3 +1,53 @@ | |||
| // 🚧 NOTE: this file exists only for the worker thread, as the worker thread needs to pass | |||
There was a problem hiding this comment.
To avoid a full lodash load can we use this eslint rule?
There was a problem hiding this comment.
Hmmm so right now we’re relying on babel-plugin-lodash to avoid full Lodash imports. Do you mean replacing babel-plugin-lodash with eslint-plugin-lodash, or using one in addition to another?
(If the former: I moderately prefer babel-plugin-lodash to ESLint rules. The former works everywhere, whereas the latter is one // eslint-disable-next-line away from bundling the full Lodash again. But if you feel this lodash-wrapper.js thingy is too much complexity, I’m happy to reconsider!)
There was a problem hiding this comment.
I would suggest using eslint-plugin-lodash in addition to babel-plugin-lodash.
If I understand correctly what is going on here, then when using the eslint-plugin-lodash, we will have to correct the imports and everything related to lodash-wrapper.js will become easier, right?
There was a problem hiding this comment.
Hmmm. So, on a second thought, I think eslint-plugin-lodash are babel-plugin-lodash aren’t really compatible:
eslint-plugin-lodash vs babel-plugin-lodash
eslint-plugin-lodash forces developers to do this change:
// Before
import _ from "lodash"
_.map()
// After
import map from "lodash/map"
map()But that’s pretty much what babel-plugin-lodash does automatically! We can either force the developers to change all imports themselves (with eslint-plugin-lodash), or we can use babel-plugin-lodash to do that automatically, but using both of them doesn’t make much sense.
Why lodash-wrapper.js exists
The reason lodash-wrapper.js exists is this:
appsmith/app/client/src/workers/common/JSLibrary/lodash-wrapper.js
Lines 49 to 61 in ea4840b
This is a workaround for a bug in babel-plugin-lodash.
If we use eslint-plugin-lodash together with babel-plugin-lodash, lodash-wrapper.js will still be needed – as babel-plugin-lodash will still be enabled and will still be introducing the bug.
If we use only eslint-plugin-lodash, lodash-wrapper.js won’t be needed anymore – we’ll be able to just // eslint-disable-next-line in the worker where we need to import full Lodash.
Tradeoffs
So, basically, here are the tradeoffs of each:
eslint-plugin-lodash |
babel-plugin-lodash |
|---|---|
| Requires the developer to change the import | Changes the import automatically |
| More explicit (= easier to debug) | More implicit (= harder to debug) |
| Easier to accidentally disable in some file | Harder to accidentally disable in some file |
| (Probably) bug-free | Has a non-obvious tricky bug with _ becoming undefined which we already stumbled upon |
Doesn’t need lodash-wrapper.js |
Needs lodash-wrapper.js as a workaround for the above bug |
I’m pretty neutral about which one to pick – which tradeoffs, do you think, matter more here?
There was a problem hiding this comment.
First of all thank you for such an answer. Excellent engineering approach! 👍
I would suggest using eslint-plugin-lodash as it is simpler and more obvious. I'm in favor of keeping things as simple as possible. So any new developer will be able to quickly figure out what's going on and make changes.
@riodeuno @SatishGandham need your comment here.
There was a problem hiding this comment.
I second @KelvinOm 's opinion on this. More explicit (= easier to debug) had me sold.
What concerns me is this: Easier to accidentally disable in some file. How do we mitigate this? Any thoughts?
There was a problem hiding this comment.
[By “Easier to accidentally disable in some file”, by the way, I’m meaning it’s easy to do this:
// eslint-disable-next-line
import _ from "lodash"If someone (e.g. an external contributor, or a new engineer who just joined the team) does this without understanding full implications of this, and it slips through the code review, it’d bump the bundle size immediately.
One way to catch this is to set up bundle size monitoring that triggers an alert if the bundle suddenly increases by X% (where X is 5-10%+). I like using SpeedCurve for that. Happy to guide you through that!]
There was a problem hiding this comment.
One way to catch this is to set up bundle size monitoring that triggers an alert if the bundle suddenly increases by X% (where X is 5-10%+). I like using SpeedCurve for that. Happy to guide you through that!]
I'm sure we should set up bundle size monitoring anyway, not just because of lodash.
There was a problem hiding this comment.
Deal! Then I’ll PR eslint-plugin-lodash as a separate PR, to avoid blocking this one 👍
iamakulov
requested review from
a team and
rahulramesha
and removed request for
a team
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5532157983. |
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5534349370. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5534349370.
|
|
↑ Very confused by all of these because
Let me re-run. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5534349370.
|
|
Still failing. I’m running a workshop on Thu so will investigate and fix on Fri 👍 |
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5558615534. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5558615534.
To know the list of identified flaky tests - Refer here |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5558615534.
To know the list of identified flaky tests - Refer here |
This ensures custom styles aren’t overridden by .uppy-u-reset from Uppy styles which is also present on the same nodes. (Right now, the order in which Uppy styles vs styled-components styles are inserted seems to be dependent only on the import structure/order.)
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5561357070. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5561357070. To know the list of identified flaky tests - Refer here |
|
↑ The issue should be fixed now! |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5558615534.
To know the list of identified flaky tests - Refer here |
1 similar comment
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5558615534.
To know the list of identified flaky tests - Refer here |
|
Okay, so it looks like my test run actually failed to run some tests (due to 429 Too Many Requests). We then re-ran the tests, but re-ran them with the outdated code – thus still including the issue I fixed in acdbc51:
To make sure we’re running the latest code from this branch, I’m going to trigger a fresh test run. |
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5568194657. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5568194657.
To know the list of identified flaky tests - Refer here |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5568194657. |
Description
This PR takes the second (and last) round on reducing the bundle size. Specifically, it:
react-json-view, and the welcome tourapp.jsonmoment-timezone(only from the main thread code) and unused code syntax highlighter definitionsThis makes the bundle another 15% smaller (5.42 MB → 4.64 MB minified). In total, since the first PR, we’ve shaved off 49% of the bundle (9.01 → 4.64 MB minified)!
Here’s the bundle before the change (with removed and code-split modules highlighted):
Note: this PR relies on appsmithorg/design-system#514 to fully code-split Uppy away.
Type of change
Testing
How Has This Been Tested?
Test Plan
Issues raised during DP testing
Checklist:
Dev activity
QA activity:
Test Plan Approvedlabel after Cypress tests were reviewedTest Plan Approvedlabel after JUnit tests were reviewed