Components: Deprecate withContext HOC and remove its usage#8189
Components: Deprecate withContext HOC and remove its usage#8189
withContext HOC and remove its usage#8189Conversation
packages/element/src/serialize.js
Outdated
| context | ||
| ); | ||
| default: | ||
| throw new Error( tagName ); |
There was a problem hiding this comment.
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.
It fails here because the mutation present in here: https://github.com/WordPress/gutenberg/pull/8189/files#diff-a72c42fcaf462e92ab6cd7a52680df76R472
|
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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
| */ | ||
| import { serialize } from '../api'; | ||
|
|
||
| const { Consumer, Provider } = createContext( { |
There was a problem hiding this comment.
I think this still needs to be updated to not set the initial context as an object, but rather the noop directly.
| * `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.
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.
Yes, makes sense. In general, we could make JSDoc validation more strict since we started using it more consistently.
fc4997d to
b6b7da7
Compare
| ); | ||
|
|
||
| expect( wrapper.root.findByType( 'div' ).children[ 0 ] ).toBe( 'ok' ); | ||
| expect( console ).toHaveWarned(); |
There was a problem hiding this comment.
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.
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.
Related docs: https://jestjs.io/docs/en/mock-functions#mocking-modules
Description
It's part of our efforts to stablize API.
How has this been tested?
npm testChecklist: