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: hydration mismatches by saving changed tokens #467

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tlgimenes
Copy link

@tlgimenes tlgimenes commented Apr 17, 2023

Hi folks!

When using twind v1 on Deno's Fresh I faced this TBT performance issue. Taking a deeper look into the performance bottleneck, I noticed we had two main performance issues.

The first one was clearing the style sheet, which causes the browser to "recalculate the style", creating a long task. To fix this issue, I just stopped clearing the sheet on resume.

The second issue is a little more subtle. Turns out that this translate function takes a long time to execute. It's usually ok when dynamically changing styles, however, when running it alongside (P)react's hydration, it can greatly increase this task's blocking time. The puzzling piece is that this part of the code should never be reached if the sheet's resume process occurs successfully. Investigating further, I noticed that tokens like md:(w-full h-full) get translated into the final html as md:w-full md:h-full, so when the tw's cache is resumed, the initial tokens are missing. To address this issue I made this cache to be resumable too by adding a new extracted json parameter that can be added as a

<script type="application/json" data-twind-cache >{ "(md:(w-full h-full))": "md:w-full md:h-full" }</script>

The nice thing about it is that it only stores the tokens where tw(token) != token, not adding any unnecessary bloat to the final html.

After applying these two performance improvements, my app's blocking time went from: 700ish ms to 150ish ms on Google's Page Speed Insights.

Another solution to this problem would be to make this translate function much faster, however I could not really understand how to do this.

Let me know if you have a better idea than shipping the tw's cache to the final html.

Thanks!

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and verify the project with pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until Twind 1.0

@tlgimenes tlgimenes requested a review from sastan as a code owner April 17, 2023 19:33
@codesandbox
Copy link

codesandbox bot commented Apr 17, 2023

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@stackblitz
Copy link

stackblitz bot commented Apr 17, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2023

⚠️ No Changeset found

Latest commit: 0cb3dd8

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

@socket-security
Copy link

New dependency changes detected. Learn more about Socket for GitHub ↗︎


🚨 Potential security issues found in this pull request. To accept the risk, merge this PR and you will not be notified again.

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

📜 Install scripts

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Package Script field Source
[email protected] (added) postinstall pnpm-lock.yaml, examples/with-sveltekit/package.json, pnpm-lock.yaml via [email protected], sites/twind.run/package.json via [email protected], sites/twind.style/package.json via [email protected]
[email protected] (added) postinstall pnpm-lock.yaml, sites/twind.run/package.json via @jspm/[email protected]
@sveltejs/[email protected] (added) postinstall pnpm-lock.yaml, examples/with-sveltekit/package.json, pnpm-lock.yaml via @sveltejs/[email protected], packages/with-sveltekit/package.json, pnpm-lock.yaml, pnpm-lock.yaml, sites/twind.run/package.json via @sveltejs/[email protected], pnpm-lock.yaml, sites/twind.style/package.json via @sveltejs/[email protected]
@swc/[email protected] (added) postinstall pnpm-lock.yaml via [email protected]
[email protected] (added) postinstall pnpm-lock.yaml, examples/with-gatsby/package.json via [email protected]
[email protected] (added) postinstall pnpm-lock.yaml, examples/with-gatsby/package.json via [email protected], [email protected], [email protected], [email protected], packages/with-gatsby/package.json via [email protected]
[email protected] (added) postinstall pnpm-lock.yaml, examples/with-gatsby/package.json via [email protected], [email protected], [email protected], [email protected], packages/with-gatsby/package.json via [email protected]
[email protected] (added) postinstall pnpm-lock.yaml, examples/with-gatsby/package.json, pnpm-lock.yaml via [email protected], [email protected], [email protected], packages/with-gatsby/package.json, pnpm-lock.yaml
[email protected] (added) postinstall pnpm-lock.yaml, examples/with-gatsby/package.json via [email protected], [email protected], [email protected], [email protected], packages/with-gatsby/package.json via [email protected]
🫣 Native code

Contains native code which could be a vector to obscure malicious code, and generally decrease the likelihood of reproducible or reliable installs.

Ensure that native code bindings are expected. Consumers may consider pure JS and functionally similar alternatives to avoid the challenges and risks associated with native code bindings.

Package Location Source
@parcel/[email protected] (added) binding.gyp pnpm-lock.yaml, examples/with-gatsby/package.json via [email protected], [email protected], [email protected], [email protected], packages/with-gatsby/package.json via [email protected]
🥩 HTTP dependency

Contains a dependency which resolves to a remote HTTP URL which could be used to inject untrusted code and reduce overall package reliability.

Publish the HTTP URL dependency to npm or a private package repository and consume it from there.

Package Dependency Location Source
[email protected] (added) node-fetch@https://registry.npmjs.org/@achingbrain/node-fetch/-/node-fetch-2.6.7.tgz package.json pnpm-lock.yaml, sites/twind.run/package.json via @jspm/[email protected]
📞 Telemetry

This package contains telemetry which tracks you.

Package Note Source
[email protected] (added) Can be disabled by setting the environment variable NEXT_TELEMETRY_DISABLED=1 pnpm-lock.yaml, examples/with-next/package.json, pnpm-lock.yaml
[email protected] (added) Can be disabled by setting the environment variable NEXT_TELEMETRY_DISABLED=1 pnpm-lock.yaml, packages/with-next/package.json, pnpm-lock.yaml
🧌 Protestware/Troll package

This package is a joke, parody, or includes undocumented or hidden behavior unrelated to its primary function.

Package Note Source
[email protected] (added) This package prints a protestware console message on install regarding Ukraine for users with Russian language locale pnpm-lock.yaml, examples/with-gatsby/package.json via [email protected], [email protected], [email protected], [email protected], packages/with-gatsby/package.json via [email protected]
Pull request alert summary
Issue Status
Install scripts ⚠️ 9 issues
Native code ⚠️ 1 issue
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ⚠️ 1 issue
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ⚠️ 2 issues
Protestware/Troll package ⚠️ 1 issue

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
[email protected] eval, network, filesystem, shell, environment +7 vercel-release-bot
[email protected] eval, network, filesystem, shell, environment +7 vercel-release-bot
@jsenv/[email protected] None +0 jsenv-admin
@sastan/[email protected] None +0 sastan
@jsenv/[email protected] None +0 jsenv-admin
[email protected] None +30 sastan
[email protected] filesystem, environment +6 nicolas.lepage
@jspm/[email protected] network, filesystem, environment +142 guybedford
[email protected] eval, filesystem, shell, environment +16 oreanno
@size-limit/[email protected] filesystem +1 ai
@size-limit/[email protected] filesystem +1 ai
[email protected] None +2 conduitry
[email protected] filesystem, shell, environment +7 dummdidumm
[email protected] filesystem, environment +6 gerrit0
@remix-run/[email protected] network, filesystem +58 mjackson
[email protected] environment +4 cwelch5
@rollup/[email protected] None +2 shellscape
@sveltejs/[email protected] eval, network, filesystem +48 svelte-admin
[email protected] eval, network, filesystem, shell, environment +24 svelte-language-tools-deploy
[email protected] None +2 lydell
[email protected] None +4 jounqin
@remix-run/[email protected] environment +146 mjackson
@remix-run/[email protected] network, filesystem, shell, environment +794 mjackson
[email protected] None +636 marvinjudehk
@sveltejs/[email protected] filesystem +16 svelte-admin
@sveltejs/[email protected] filesystem, environment +13 svelte-admin
[email protected] network, filesystem, environment +1264 pieh
[email protected] environment +3 gnoff
[email protected] None +6 fredkschott
@vitest/[email protected] filesystem, environment +22 oreanno
@typescript-eslint/[email protected] None +7 jameshenry
@sveltejs/[email protected] network, filesystem, environment +29 svelte-admin
[email protected] environment +5 gaearon
[email protected] network, filesystem, environment +675 marvinjudehk
[email protected] filesystem, environment +8 tgreyuk
@remix-run/[email protected] network, environment +10 mjackson
[email protected] filesystem +633 marvinjudehk
@rollup/[email protected] filesystem +9 shellscape
[email protected] network, environment +0 alexandrudima
[email protected] environment +0 dummdidumm
[email protected] None +0 prettier-bot
@typescript-eslint/[email protected] None +13 jameshenry
[email protected] filesystem, shell, environment +5 typedoc-bot

🚮 Removed packages: @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], @twind/[email protected], [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant