-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Deprecate withContext
HOC and remove its usage
#8189
Conversation
packages/element/src/serialize.js
Outdated
@@ -445,6 +445,8 @@ export function renderElement( element, context = {} ) { | |||
}, | |||
context | |||
); | |||
default: | |||
throw new Error( tagName ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn' make it throw so I figured out we don't use the source code in all cases. It turned out it is even worse, we don't use in a majority of cases. I opened #8188 to fix it before I can continue debugging.
76b5cdc
to
12793b1
Compare
</Consumer> | ||
</Provider> | ||
{ '|' } | ||
<Consumer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails here because the mutation present in here: https://github.com/WordPress/gutenberg/pull/8189/files#diff-a72c42fcaf462e92ab6cd7a52680df76R472
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's no longer an issue.
Almost there, I made all the tests pass, which is very promising. When comparing the current implementation with |
12793b1
to
88e833c
Compare
I took advantage of the fact we use recursion and add another param which transports values from the providers created with new Context API: 88e833c It needs some polishing but it solves all the issues we had. |
BlockContentProvider.childContextTypes = { | ||
BlockContent: () => {}, | ||
return ( | ||
<Provider value={ { BlockContent } }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pass a new value each render. I suppose this isn't a huge issue since it's used only in the serialization step anyways, but why does the context need to be an object anyways? Why not just the BlockContent
value directly?
* | ||
* @return {Component} Enhanced component with injected context as props. | ||
*/ | ||
export const withBlockContentContext = ( mapContextToProps ) => createHigherOrderComponent( ( OriginalComponent ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my previous point, I don't see why we'd be passing anything other than the content component. We shouldn't need to map anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it for consistency with other context related HOCs. However, you made a good point. I will refactor as suggested.
8ef02fb
to
fc4997d
Compare
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { serialize } from '../api'; | ||
|
||
const { Consumer, Provider } = createContext( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still needs to be updated to not set the initial context as an object, but rather the noop directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, missed this one :(
/** | ||
* An internal block component used in block content serialization to inject | ||
* nested block content within the `save` implementation of the ancestor | ||
* component in which it is nested. The component provides a pre-bound | ||
* `BlockContent` component via context, which is used by the developer-facing | ||
* `InnerBlocks.Content` component to render block content. | ||
* | ||
* @return {WPElement} Element with BlockContent injected via context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: We might want to establish conventions around tag order. How I've treated it, I always have @return
last, preceded by @param
, preceded by anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense. In general, we could make JSDoc validation more strict since we started using it more consistently.
fc4997d
to
b6b7da7
Compare
@@ -39,6 +39,7 @@ describe( 'withContext', () => { | |||
); | |||
|
|||
expect( wrapper.root.findByType( 'div' ).children[ 0 ] ).toBe( 'ok' ); | |||
expect( console ).toHaveWarned(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Question came up from @danielbachhuber about testing with deprecated warnings, and potential for conflicts with its internal memoization. Made me think over the weekend that the fact we test against console logging is a bit of an abstraction piercing; while it seems obvious deprecated
would log to console by its description, we can't know for certain, and seems we should test that deprecated
was merely called, regardless of its internal implementation.
I'd like to think this would be as simple as? ...
const deprecated = jest.mock( '@wordpress/deprecated' );
// ...
expect( deprecated ).toHaveBeenCalled();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should need:
import deprecated from '@wordpress/deprecated';
jest.mock( '@wordpress/deprecated', () => jest.fn() );
expect( deprecated ).toHaveBeenCalled();
I can give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related docs: https://jestjs.io/docs/en/mock-functions#mocking-modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in cf41540.
Description
It's part of our efforts to stablize API.
How has this been tested?
npm test
Checklist: