Skip to content
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

fix: reduce toolbar bundle size by 60% #20122

Merged
merged 67 commits into from Feb 19, 2024
Merged

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Feb 4, 2024

Toolbar is 2.2MB of JS, that's an awful lot...

a production build shows half a meg of syntax highlighting

Screenshot 2024-02-04 at 20 00 12

  • outputs a file that can be used to generate a view of the bundle at https://esbuild.github.io/analyze/ (that has awesome explanation of why a file is included when you click on things)
  • also allows outputting a HTML file of the bundle locally NODE_ENV=production pnpm build && pnpm visualize-toolbar-bundle
  • moves LemonMarkdown out of the LemonTextArea file so that a circular dependency out to TextCard and back doesn't affect LemonTextArea
  • declares which files have sideeffects so that we can tree-shake LemonUI in the toolbar bundle
  • and removing CodeSnippet for showing the current selector in the HTML element window
  • and replaces any import of posthog-js with posthog-js-lite saving about 90kb
  • and makes sure we don't use the datepicker which removes a few dependencies
  • down by 60% at this point
  • almost all remaining is react and antd
Screenshot 2024-02-17 at 22 07 16

Copy link
Contributor

github-actions bot commented Feb 4, 2024

Size Change: -1.37 MB (-61%) 🏆

Total Size: 863 kB

Filename Size Change
frontend/dist/toolbar.js 863 kB -1.37 MB (-61%) 🏆

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

6 snapshot changes in total. 0 added, 6 modified, 0 deleted:

  • chromium: 0 added, 6 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@pauldambra
Copy link
Member Author

@Twixes the toolbar is bundling more of lemon ui than I think it should... which means it's getting half a meg of formatting code it will never use.

There don't seem to be any tools to investigate this other than thinking 😱

Tagging you since it's lemon ui... and in case you've any suggestions on how to investigate 😊

@Twixes
Copy link
Collaborator

Twixes commented Feb 6, 2024

Oh wow, we definitely don't need a lot of this. @pauldambra By "investigate" do you mean finding out why this is not being tree-shaked? It definitely should be, but I have never looked into how we do tree-shaking at all, so would have to start with reading into that part of our ESBuild process 😅

@pauldambra
Copy link
Member Author

I have never looked into how we do tree-shaking at all

Ah, that makes two of us 🙈

You're welcome to ignore this then and I'll keep it spinning in the background

@pauldambra pauldambra added waiting Prevents stale-bot from marking the PR as stale. and removed stale labels Feb 14, 2024
@pauldambra
Copy link
Member Author

if i edit the toolbar so that all it does is import lemontextarea then we still import the half meg of formatting code 👀

Screenshot 2024-02-17 at 13 08 39

@PostHog PostHog deleted a comment from posthog-bot Feb 17, 2024
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

756 snapshot changes in total. 0 added, 756 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

42 snapshot changes in total. 0 added, 42 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 0 modified, 0 deleted
  • webkit: 0 added, 1 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@pauldambra pauldambra changed the title fix: toolbar bundle size fix: reduce toolbar bundle size by 60% Feb 17, 2024
@pauldambra pauldambra requested review from Twixes and a team February 17, 2024 22:04
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 0 modified, 0 deleted
  • webkit: 0 added, 1 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@marandaneto
Copy link
Member

LGTM but I don't feel confident enough to YOLO, maybe someone else with more experience on the codebase + bundlers can take a look as well.

Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Any idea where the remaining antd.less styling is coming from? Based on this comment it suggests we removed it from the toolbar at some point

@pauldambra
Copy link
Member Author

Any idea where the remaining antd.less styling is coming from? Based on this comment it suggests we removed it from the toolbar at some point

ah, no, is a dangling comment from me getting over excited in the past 🙈

@pauldambra pauldambra merged commit d5df038 into master Feb 19, 2024
79 checks passed
@pauldambra pauldambra deleted the fix/toolbar-bundle-size branch February 19, 2024 11:39
@daibhin
Copy link
Contributor

daibhin commented Feb 19, 2024

Ah ok, maybe we can remove it though 🤔 I'll have a look where we're still relying on antd in the toolbar because it's not imported directly anywhere in that directory

@pauldambra
Copy link
Member Author

I think in the tooltip component still?

We use lemon-ui, so while we're still using antd there then I think we're stuck...

@daibhin
Copy link
Contributor

daibhin commented Feb 19, 2024

Another reason to get this PR in #20160 💪

@pauldambra
Copy link
Member Author

@joethreepwood in case you do changelog before wednesday all hands this one should definitely go in

thmsobrmlr pushed a commit that referenced this pull request Feb 20, 2024
* output analyzable build info for the toolbar

* don't use code snippet it adds half a meg

* Update UI snapshots for `chromium` (2)

* use esbuild visualizer instead

* fix

* allow treeshaking and remove circular dependency from imports toolbar uses

* fix

* lint the mjs files at the root of frontend folder

* no need to mention lemonui at all

* no ned to specify metafile

* don't allow posthog-js to sneak into the toolbar

* simpler date picker so fewer dependencies

* maybe this

* like this?

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* ragE

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* too easy to break things this way

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* around the houses

* Reset snapshots to master

* explain why there's a plugin

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* fix

* fix

* Update UI snapshots for `webkit` (2)

* fix

* Update UI snapshots for `webkit` (2)

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Prevents stale-bot from marking the PR as stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants