Theme: Generate default ramps at compile time #72847
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aduth
commented
Oct 30, 2025
| import glob from 'glob'; | ||
| import { dirname, basename } from 'path'; | ||
| import stripJsonComments from 'strip-json-comments'; | ||
| import JSONC from 'jsonc-parser'; |
Member
Author
There was a problem hiding this comment.
tsconfig files are JSONC files, so we may as well use a JSONC parser than trying to transform the string into something we can shove into JSON.parse. This is the library that TypeScript itself maintains and uses internally.
aduth
commented
Oct 30, 2025
package.json
Outdated
| "deep-freeze": "0.0.1", | ||
| "equivalent-key-map": "0.2.2", | ||
| "esbuild": "0.25.10", | ||
| "esbuild-esm-loader": "0.3.3", |
Member
Author
There was a problem hiding this comment.
This is a package that I maintain and have used in a number of other projects. It itself is pretty light-weight and delegates most of the underlying work to Node's module customization hooks and ESBuild.
Contributor
There was a problem hiding this comment.
Why is it added to the top level package instead of the theme package where it's used?
Member
Author
There was a problem hiding this comment.
I'm considering it a devDependencies and as far as I recall we define all of those in the root package.json?
Contributor
There was a problem hiding this comment.
It is possible that how things started, but I'm thinking that ideally going forward, each package should define its own dependencies both prod and dev. Unless the dependency is used outside packages. (bin for instance).
Not a blocker for this PR, something we can iterate on later.
Member
Author
There was a problem hiding this comment.
I think that makes sense, especially here where it is a devDependency (relevant for compile-time), but only for this package, and not repo-wide. Happy to make the change.
Member
Author
There was a problem hiding this comment.
The tooling is set up to disallow it, so I'll have to make some revisions there.
✖ prefer-no-devDependencies - node: devDependencies - devDependencies should not be defined
Member
Author
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -22 B (0%) Total Size: 2.38 MB
ℹ️ View Unchanged
|
aduth
added a commit
that referenced
this pull request
Oct 31, 2025
Makes it clearer which dependency is relevant for which packages. Dev dependencies which are "repo-wide" (i.e. used in root-level scripts) should still be defined in the root package.json
72ad4d1 to
8352a52
Compare
jsnajdr
approved these changes
Nov 3, 2025
| > = { | ||
| bg: { | ||
| ramp: { | ||
| surface2: { color: '#f8f8f8', warning: false }, |
Member
There was a problem hiding this comment.
In near future, we could eliminate the warning field. It's ever used only by Storybook to mark colors that fail to meet the contrast requirements.
In practice, we always adjust the seed color so that the entire ramp fits within the 0.0-1.0 lightness range, with all contrast requirements met. The warning flag is always false unless you're doing some debugging.
Member
Author
There was a problem hiding this comment.
In near future, we could eliminate the
warningfield. It's ever used only by Storybook to mark colors that fail to meet the contrast requirements.In practice, we always adjust the seed color so that the entire ramp fits within the 0.0-1.0 lightness range, with all contrast requirements met. The
warningflag is alwaysfalseunless you're doing some debugging.
Yeah I think we could do a few things to condense the shape of this object so it doesn't take up so much space.
Even if we want warning as is, we could maybe have it as an (optional) adjacent key of the ramp so that each of these ramp values is just the string hex value rather than an object.
e.g.
bg: {
ramp: {
surface2: '#f8f8f8',
/* ... */
},
direction: 'darker',
/* warnings: [ 'surface2' ] */
}
This was referenced Nov 3, 2025
peterwilsoncc
pushed a commit
to peterwilsoncc/gutenberg-build
that referenced
this pull request
Nov 3, 2025
* Update tsconfig validator to allow multi-config packages * Theme: Generate default ramps at compile time * Allow package-level devDependencies Makes it clearer which dependency is relevant for which packages. Dev dependencies which are "repo-wide" (i.e. used in root-level scripts) should still be defined in the root package.json * Move esbuild-esm-loader as devDependency of theme package See: WordPress/gutenberg#72847 (comment) Source: WordPress/gutenberg@8474a11
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Updates theme package build to generate default ramps at compile time.
It also includes a few other related framework adjustments:
packages/theme/bintsconfig*.jsonin a package.tsfiles in Node.js usingesbuild-esm-loader. This should be a temporary solution until we migrate to a version of Node.js supporting native type-stripping (see related discussion), and is intended to integrate with our existing ESBuild toolchain.Why?
The default experience will use these ramps. For default values, we can know ahead of time the results of the built ramps, so we can precompute these and avoid the added runtime cost.
How?
Build script now generates default ramps.
Testing Instructions
Validate no regressions in behavior of behavior of theme provider:
npm run storybook:startVerify that building theme package succeeds and produces no local changes:
npm run --workspace @wordpress/theme buildgit status