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.

Sign up for GitHub

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

Jump to bottom

Interactivity API: Remove non default suffix data wp context processing. #58664

Merged
merged 6 commits into from Feb 7, 2024
Merged

Interactivity API: Remove non default suffix data wp context processing. #58664

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5eee96a
Add test that should fail without the fix
cbravobernal Feb 2, 2024
48773fa
Prevent processing data-wp-context-whatever
cbravobernal Feb 5, 2024
3b6d1d9
Revert remove whitespace
cbravobernal Feb 5, 2024
7039c38
Update changelog
cbravobernal Feb 5, 2024
c4a658b
Add default test
cbravobernal Feb 5, 2024
b68c44c
Refactor function as requested
cbravobernal Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php
View file
Open in desktop
Expand Up @@ -134,3 +134,10 @@
<button data-testid="navigate" data-wp-on--click="actions.navigate">Navigate</button>
<button data-testid="async navigate" data-wp-on--click="actions.asyncNavigate">Async Navigate</button>
</div>

<div
data-wp-interactive='{"namespace": "directive-context-non-default"}'
data-wp-context--non-default='{ "text": "non default" }'
DAreRodz marked this conversation as resolved.
Show resolved Hide resolved
>
<span data-testid="non-default suffix context" data-wp-text="context.text"></span>
</div>
4 changes: 4 additions & 0 deletions packages/interactivity/CHANGELOG.md
View file
Open in desktop
Expand Up @@ -6,6 +6,10 @@

- Break up init with yielding to main to prevent long task from hydration. ([#58227](https://github.com/WordPress/gutenberg/pull/58227))

### Bug fixes

- Interactivity API: Remove non default suffix data wp context processing. ([#58664](https://github.com/WordPress/gutenberg/pull/58664))

## 4.0.1 (2024-01-31)

### Bug Fixes
Expand Down
29 changes: 18 additions & 11 deletions packages/interactivity/src/directives.js
View file
Open in desktop
Expand Up @@ -102,17 +102,24 @@ export default () => {
const inheritedValue = useContext( inheritedContext );
const currentValue = useRef( deepSignal( {} ) );
const passedValues = context.map( ( { value } ) => value );

currentValue.current = useMemo( () => {
const newValue = context
.map( ( c ) => deepSignal( { [ c.namespace ]: c.value } ) )
.reduceRight( mergeDeepSignals );

mergeDeepSignals( newValue, inheritedValue );
mergeDeepSignals( currentValue.current, newValue, true );
return currentValue.current;
}, [ inheritedValue, ...passedValues ] );

try {
const { namespace, value } = context.find(
( { suffix } ) => suffix === 'default'
);
DAreRodz marked this conversation as resolved.
Show resolved Hide resolved
currentValue.current = useMemo( () => {
const newValue = deepSignal( {
[ namespace ]: value,
} );
mergeDeepSignals( newValue, inheritedValue );
mergeDeepSignals( currentValue.current, newValue, true );
return currentValue.current;
}, [ inheritedValue, ...passedValues ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't expect useMemo() to throw an error. Moving this outside of the try { ... } catch would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, if this is intentional, then we need to include a test for that case, don't we? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added previously cause there was errors of trying to map an undefined variable.
As we are now removing:

.map( ( c ) => deepSignal( { [ c.namespace ]: c.value } ) )
					.reduceRight( mergeDeepSignals );

That possible error has gone. Now it works fine with two contexts. As you mentioned to add to the test.

} catch ( e ) {
// If there's an error, we'll just return the inherited context.
return (
<Provider value={ inheritedValue }>{ children }</Provider>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to render another Provider as the inherited value already comes from a Provider. Actually, we can skip this part entirely because the directives don't need to return anything. 🤓

Copy link
Contributor

Choose a reason for hiding this comment

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

An empty return is enough.

}
return (
<Provider value={ currentValue.current }>{ children }</Provider>
);
Expand Down
8 changes: 8 additions & 0 deletions test/e2e/specs/interactivity/directive-context.spec.ts
View file
Open in desktop
Expand Up @@ -189,4 +189,12 @@ test.describe( 'data-wp-context', () => {
await page.getByTestId( 'async navigate' ).click();
await expect( element ).toHaveText( 'changed from async action' );
} );
test( 'should bail out if the context is not a default directive', async ( {
page,
} ) => {
// This test is to ensure that the context directive is only applied to the default directive
// and not to any other directive.
const element = page.getByTestId( 'non-default suffix context' );
await expect( element ).toHaveText( '' );
DAreRodz marked this conversation as resolved.
Show resolved Hide resolved
} );
} );