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

Interactivity: Prevent empty state or config from being printed as [] #6261

Closed

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Mar 12, 2024

Prevent an empty Interactivity store from being serialized as []. Stores are expected to be JSON objects ({}), but PHP will json_encode empty arrays as [].

An option would be to force PHP to use empty objects with JSON_FORCE_OBJECT, but that is deep, so if we wanted e.g. array( 'myStore' => array( 'ids' => array() ) ) the inner array() would also become {}, which we probably don't want.

Instead, this PR prunes stores and configurations that are empty arrays. This results in less redundant data being serialized into the HTML and fixes the issue of store objects being serialized as JavaScript arrays.

Trac ticket: https://core.trac.wordpress.org/ticket/60761


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

github-actions bot commented Mar 12, 2024

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, luisherranz, darerodz, gziolo, swissspidy.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@@ -140,20 +140,36 @@ public function config( string $store_namespace, array $config = array() ): arra
* @since 6.5.0
Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect this is late for 6.5, in that case an annotation should probably be added here that empty state objects are not printed.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... it would be safer to print them, but always using objects to mimic JS, don't you think?

Something like this:

function convert_empty_arrays_to_objects($data) {
    if (is_array($data)) {
        if (empty($data)) {
            return new stdClass();
        }
        
        foreach ($data as $key => $value) {
            $data[$key] = convert_empty_arrays_to_objects($value);
        }
    }
    
    return $data;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an alternative. Why would it be safer? The client will use {} by default if no server provided state or config are provided, won't it?

Copy link
Member Author

@sirreal sirreal Mar 13, 2024

Choose a reason for hiding this comment

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

The relevant client code is here:

export const parseInitialData = ( dom = document ) => {
        const storeTag = dom.querySelector(
                `script[type="application/json"]#wp-interactivity-data`
        );
        if ( storeTag?.textContent ) {
                try {
                        return JSON.parse( storeTag.textContent );
                } catch ( e ) {
                        // Do nothing.
                }
        }
        return {};
};

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 This can actually change behavior right now if we omit stores. Subscriptions are slightly buggy if the store is not initialized before hydration. That's an edge case I hope to address in WordPress/gutenberg#59842.

If that PR lands with the proposed approach, it seems preferable to avoid adding unnecessary bytes to the page to declare empty stores and configs.

Choose a reason for hiding this comment

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

I think the fix is good. I would only prevent rendering an empty array when there's neither state nor config to include in the <script> tag. In addition, it would also save some bytes. 🙂

For arrays inside the state/config, that's something developers could handle on their own.

In any case, this fix is not critical, am I right? I mean, this is the function that initializes the store:

https://github.com/WordPress/gutenberg/blob/b06fcb1a7d4b3ae3b183625d7a1ae9a191e17ff7/packages/interactivity/src/store.ts#L327-L330

If an empty array were present, it would destructure the state and config properties, obtaining undefined for both.

Copy link
Member

Choose a reason for hiding this comment

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

Disregard my previous comment. It should be up to the developer to indicate what will end up as an array and what will end up as an object:

wp_interactivity_state( 'myPlugin', array(
  'array' => array(),
  'obj'   => (object) array(),
));

Copy link
Member Author

@sirreal sirreal Mar 14, 2024

Choose a reason for hiding this comment

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

In any case, this fix is not critical, am I right? I mean, this is the function that initializes the store

Ah, I hadn't looked at that code. Here it is:

export const populateInitialData = ( data?: {
        state?: Record< string, unknown >;
        config?: Record< string, unknown >;
} ) => {
        if ( isObject( data?.state ) ) {
                Object.entries( data.state ).forEach( ( [ namespace, state ] ) => {
                        store( namespace, { state }, { lock: universalUnlock } );
                } );
        }
        if ( isObject( data?.config ) ) {
                Object.entries( data.config ).forEach( ( [ namespace, config ] ) => {
                        storeConfigs.set( namespace, config );
                } );
        }
};

EDIT: The rest of this comment is inaccurate. I'll provide another comment.

If the server provided initial data state is not an object then the store is not created at all. Again, this is related to WordPress/gutenberg#59842 which will ensure that stores exist. I'd also consider adding a store to the client if invalid data was provided (I can add that to WordPress/gutenberg#59842).

If WordPress/gutenberg#59842 lands, this fix is entirely an optimization.

Copy link
Member Author

@sirreal sirreal Mar 14, 2024

Choose a reason for hiding this comment

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

My previous comment is wrong. This is still an important fix. Why? Because state is always expected to be an object. If we review the client's behavior, it parses the server provided data which has this form:

{
  state: {
    [namespace]: stateValueForNamespace,
    // …etc
  },
  config: {
    [config]: configValueForNamespace,
    // …etc
  },
}

But the client only looks at the top-level state and config object and skips store creation (or config set) if that level is not a plain object. Otherwise, it sets whatever is provided. Therefore, when the server does send arrays as state objects the client is not well behaved:

{ "state": {
  "badState": [],
  "okState": {},
} }

There don't seem to be other plain object checks in the store function on the client.

The entire code block
export const parseInitialData = ( dom = document ) => {
	const storeTag = dom.querySelector(
		`script[type="application/json"]#wp-interactivity-data`
	);
	if ( storeTag?.textContent ) {
		try {
			return JSON.parse( storeTag.textContent );
		} catch ( e ) {
			// Do nothing.
		}
	}
	return {};
};

export const populateInitialData = ( data?: {
	state?: Record< string, unknown >;
	config?: Record< string, unknown >;
} ) => {
	if ( isObject( data?.state ) ) {
		Object.entries( data.state ).forEach( ( [ namespace, state ] ) => {
			store( namespace, { state }, { lock: universalUnlock } );
		} );
	}
	if ( isObject( data?.config ) ) {
		Object.entries( data.config ).forEach( ( [ namespace, config ] ) => {
			storeConfigs.set( namespace, config );
		} );
	}
};

// Parse and populate the initial state and config.
const data = parseInitialData();
populateInitialData( data );

Choose a reason for hiding this comment

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

Oh, right. Yup, this fix seems relevant. 😅

@sirreal
Copy link
Member Author

sirreal commented Mar 12, 2024

👋 @c4rl0sbr4v0 @DAreRodz for review.

@sirreal
Copy link
Member Author

sirreal commented Mar 13, 2024

This pairs nicely with WordPress/gutenberg#59842 which will ensure stores are initialized on the client before they're needed.

Copy link

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

LGTM!

@sirreal sirreal requested a review from gziolo March 14, 2024 15:14
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This looks good to me. It has now improved test coverage covering several edge cases.

@swissspidy
Copy link
Member

Committed in https://core.trac.wordpress.org/changeset/57841

@swissspidy swissspidy closed this Mar 15, 2024
@sirreal sirreal deleted the fix/interactivity-api-serialize-object branch March 15, 2024 12:14
sirreal added a commit to WordPress/gutenberg that referenced this pull request Mar 15, 2024
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.

5 participants