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

Handle an evolving theme.json shape (consider add versioning). #27230

Closed
jorgefilipecosta opened this issue Nov 24, 2020 · 15 comments
Closed

Handle an evolving theme.json shape (consider add versioning). #27230

jorgefilipecosta opened this issue Nov 24, 2020 · 15 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json

Comments

@jorgefilipecosta
Copy link
Member

This needs exploration. One option is that theme.json files have a version number, similarly to block.json files. so if we need to make a breaking change we can still keep code that is able to deal with files from a previous version.

@jorgefilipecosta jorgefilipecosta added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Nov 24, 2020
@oandregal
Copy link
Member

If we follow the v1, v2 version mechanism, what should happen when we introduce a new version? Should the old version be discarded? Or do we need to support all the existing versions?

My understanding is that we need to support all existing versions, is that correct?

@oandregal
Copy link
Member

I ran this one by @youknowriad and he mentioned this shouldn't be a blocker for #27506 We can cross this bridge when we get to it, the same we did for the block versions. cc @jorgefilipecosta for thoughts.

@jorgefilipecosta
Copy link
Member Author

Hi @nosolosw,

Or do we need to support all the existing versions?

We need to support all the versions. But having a version allows to change the meaning of things e.g: the meaning of something in v2 can be different from the meaning in v1.

I ran this one by @youknowriad and he mentioned this shouldn't be a blocker for #27506

Including the version field itself may not be a blocker, but our code has problems with just adding a version field.
The current theme.json shape is

{
'global': ..
'core/paragraph': ...
}

Our code assumes everything is a style context. So if we pass something like

{
'version': ...
'core/paragraph': ...
'global': ...
}

Our code breaks and does not work as expected.

Besides the version, we will also need things like Theme names and other meta fields in the future.
Having a version as a sibling of contexts also does not makes much sense.
I wonder if we should include a new property that is a root property for all the contexts in theme.json? cc: @nosolosw, @youknowriad
eg:

{
'version': ...
'contexts':{
	'core/paragraph': ...
	'global': ...
}

or

{
'version': ...
'definitions':{
	'core/paragraph': ...
	'global': ...
}

I think we may not have versions, for now, but if someone passes a theme.json with a version field our code should still work and behave correctly by just ignoring that field.

@youknowriad
Copy link
Contributor

I do think we need a top level property but "definitions" and "contexts" are very weird. Why don't we have "style" and "config" for instance?

@oandregal
Copy link
Member

An alternative I like more is to reserve some "context" name for this kind of metadata, that our code skips when it finds it. For example, we could use theme-json/metadata (a very unlikely block name) as something to be skipped in context processing, so:

{
  'theme-json/metadata': { metadata like version, etc goes here },
  'global': { ... },
  'core/paragraph': { ... }
}

@jorgefilipecosta
Copy link
Member Author

I do think we need a top-level property but "definitions" and "contexts" are very weird. Why don't we have "style" and "config" for instance?

Well inside each context we already have a "style" I property. "config" seems fine to me.

An alternative I like more is to reserve some "context" name for this kind of metadata, that our code skips when it finds it. For example, we could use theme-json/metadata (a very unlikely block name) as something to be skipped in context processing, so:

{
  'theme-json/metadata': { metadata like version, etc goes here },
  'global': { ... },
  'core/paragraph': { ... }
}

In this approach in every place in code where we need to iterate no the contexts, we need to be aware that 'theme-json/metadata' needs to be skipped. It seems more natural to me to have a root property.

@youknowriad
Copy link
Contributor

Well inside each context we already have a "style" I property. "config" seems fine to me.

I guess what I was suggesting is inverting the nesting style: [ context ]: { something } and config: [ context ]: { something }

@oandregal
Copy link
Member

I've just seen #27778 where Riad is adding the custom templates' title to theme.json (so it can be translated to users, hence also relevant for wp-cli/i18n-command#224). With this new information, the suggestion of having styles and config as top-level keys, and the conversations in scattered PRs with theme people wanting to add more things to theme.json (theme metadata, etc), I agree that contexts should no longer be the top-level keys of theme.json.

@oandregal
Copy link
Member

I've started a PR to make settings & styles top-level keys at #28110

@oandregal
Copy link
Member

Given the current shape of the theme.json it looks like it'll suffice to have a new top-level field called "version" in the theme.json. If none is present the code that processes this will assume is the first one, so I'm sure we need to do anything here for the first version ― subsequent updates should check for the required version instead.

@oandregal
Copy link
Member

#30541 has landed, which introduced a version field in the theme.json and a mechanism to migrate to the latest schema in the server. The corresponding migration mechanism in the client is to be developed.

@youknowriad
Copy link
Contributor

Removed from the 5.8 project. The client upgrades are not required there.

@oandregal
Copy link
Member

Removed from the 5.8 project. The client upgrades are not required there.

Oh, good call. Moved this to the backlog of #20331 (removed from current iteration).

@grappler
Copy link
Member

grappler commented Jan 4, 2022

I think the decision has been made to follow a versioning that matches the WordPress releases. Currently the "version 1" can be found in https://schemas.wp.org/wp/5.8/theme.json and for WordPress 5.9 the schema can be found here: https://schemas.wp.org/wp/5.9/theme.json

https://make.wordpress.org/themes/2021/11/30/theme-json-schema/

@oandregal
Copy link
Member

Closing this as we now gracefully support two versions of theme.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

No branches or pull requests

4 participants