emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.43k stars 1.11k forks source link

[perf] Remove `Insertion` wrapper #3243

Closed romgrk closed 1 month ago

romgrk commented 1 month ago

I'm not sure if there is a good reason to keep the Insertion wrapper that is created by the styled function. It creates additional object allocations (JSX & props) and creates more work for React (more vdom diffing).

I've removed it in this PR and benchmarked again with the same test case I've been using, here are the results:

Before: 104.2ms +- 7.0
After:  99.6    +- 6.7

N = 20
changeset-bot[bot] commented 1 month ago

⚠️ No Changeset found

Latest commit: aa227fb9af17722420673f725058846b1cb739c3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codesandbox-ci[bot] commented 1 month ago

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.

Andarist commented 1 month ago

Unfortunately, it exists for a reason. It would be a breaking change to remove it - we can explore this in the next major version that will target React 19 but I don't see us doing that right now. While users shouldn't rely on insertion order... they absolutely do that today. I'd expect many MUI users would be dissatisfied with this change in a minor bump.

romgrk commented 1 month ago

Gotcha, I'll explore other improvements if this one is impossible. I'm curious though, would you mind explaining why it's required? I figured that if I inlined the component at the same place in the tree where it was previously rendered, the side-effects would run in the same order as they previously would.

Andarist commented 1 month ago

I figured that if I inlined the component at the same place in the tree where it was previously rendered, the side-effects would run in the same order as they previously would.

I might have screwed to replicate the exact structure that we are using here but I feel it's pretty close to it here: https://stackblitz.com/edit/vitejs-vite-egfmw1?file=src%2FApp.tsx,src%2Fmain.tsx&terminal=dev

Notice how the parent's side effect is either executed before or after the side-effects from its children (based on the used insertion technique)

romgrk commented 1 month ago

Makes sense, thanks for the details :+1: But yeah it's terrible for performance and DX (pollutes the react devtools), would be nice to remove it in the next major.