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

Add precompile step for colorSchemes #4337

Merged
merged 5 commits into from
Mar 11, 2024
Merged

Conversation

manuelpuyol
Copy link
Contributor

Closes #

#4336

Changelog

Precompile colorSchemes to make load times faster.

New

  • Added a precompile-color-schemes build step that will output a .ts file with the precompiled object.

Removed

  • Removed colorSchemes compilation step from runtime

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Mar 1, 2024

🦋 Changeset detected

Latest commit: 5ff242b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Contributor

github-actions bot commented Mar 1, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 87.5 KB (-1.76% 🔽)
packages/react/dist/browser.umd.js 87.83 KB (-1.64% 🔽)

@siddharthkp
Copy link
Member

This looks promising!

Let's test it with gh/gh for any accidental errors

@manuelpuyol manuelpuyol marked this pull request as ready for review March 5, 2024 17:19
@manuelpuyol manuelpuyol requested review from a team and joshblack March 5, 2024 17:19
@manuelpuyol
Copy link
Contributor Author

manuelpuyol commented Mar 6, 2024

Bundle diff

before:

Stat: 309kb
Parsed: 277kb
Gzippecd: 22.42kb

Image showing bundle sizes listed above

after:

Stat: 325kb (+16kb)
Parsed: 269kb (-8kb)
Gzippecd: 19.42kb (-3kb)

Image showing bundle sizes listed above

The raw bundle is slightly bigger, but smaller when Parsed or Gzipped

@primer primer deleted a comment from github-actions bot Mar 6, 2024
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to do this! Really appreciate you exploring this to improve performance in this area.

One question I had was if it was possible to statically update src/legacy-theme/ts/colors/index.ts into the intended format that the precompile step is taking or if this is something that will always need to execute, whether that's on load or at a precompile step.

@manuelpuyol
Copy link
Contributor Author

manuelpuyol commented Mar 7, 2024

hey @joshblack technically we don't need the compile step, we could just copy the output and put it in a static file. If those colors rarely change I think that could be a good idea.

I made it a compile step simply because I don't know how often they change and I thought it could be easier for a developer to edit the base files and then the compile step taking care of converting everything into a big object

@primer primer deleted a comment from github-actions bot Mar 7, 2024
@manuelpuyol manuelpuyol requested a review from a team as a code owner March 11, 2024 16:32
@manuelpuyol manuelpuyol requested a review from pksjce March 11, 2024 16:32
@manuelpuyol manuelpuyol added this pull request to the merge queue Mar 11, 2024
Merged via the queue into main with commit 69f4489 Mar 11, 2024
30 checks passed
@manuelpuyol manuelpuyol deleted the mp/precompile-color-scheme branch March 11, 2024 17:04
@primer primer bot mentioned this pull request Mar 11, 2024
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
* Add precompile step for colorSchemes

* ignore generated file in prettier

* Create tasty-spoons-suffer.md
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.

3 participants