Jetpack: Blocks: broken with WP 5.3
16107 may have broken our blocks in older versions of WordPress.
If you test a WordPress 5.3.4 site with Jetpack Master today, you'll run into the following error when trying to insert an Image Compare block for example:
TypeError: Object(...) is not a function
at eval (cover-media-placeholder.js:75)
@retrofox Do you think you could take a look?
jeherve
All 14 comments
Assigning @marekhrabe who beat me to that report here:
https://github.com/Automattic/jetpack/pull/16069#issuecomment-647611158
jeherve
on 22 Jun 2020
I'm working on a fix. The most straightforward path for me was to introduce a new HOC in Jetpack that deals with the compatibility differences. I've created withBlockName, since that is the only piece of the context we use.
/**
* WordPress dependencies
*/
import { useBlockEditContext, withBlockEditContext } from '@wordpress/block-editor';
import { createHigherOrderComponent } from '@wordpress/compose';
const withBlockName = createHigherOrderComponent(
( WrappedComponent ) => ( props ) => {
// Use hook available in WP 5.4 and newer.
if ( typeof useBlockEditContext === 'function' ) {
const { name } = useBlockEditContext();
return <WrappedComponent { ...props } blockName={ name } />;
}
// Fallback to HOC for versions before 5.4.
return withBlockEditContext( ( { name } ) => ( { blockName: name } ) )(
WrappedComponent
);
},
'withBlockName'
);
export default withBlockName;
However, I have just hit a roadblock :( While useBlockEditContext is exported from the block editor package, the predecesor withBlockEditContext was never exported and only used internally in Gutenberg. That means my code above won't work. Investigating more…
marekhrabe
on 22 Jun 2020
Both versions are exported from context.js, which means they could be imported and used in the Gutenberg codebase, however, only the newer useBlockEditContext is reexported and available in the API plugins can use.
Context definition in WP 5.3:
Context definition in WP 5.4:
Public export in 5.3:
Public export in 5.4:
marekhrabe
on 22 Jun 2020
@retrofox Do you think you could take a look?
yes, of course
retrofox
on 22 Jun 2020
From what I found so far, I don't think there is a suitable alternative for useBlockEditContext before WP 5.4. Since we are only using it in different places where we alter the MediaPlaceholder using the editor.MediaPlaceholder.
I've tried inspecting props of the MediaPlaceholder to see if we can determine the block name some other way. In WP 5.3, the className prop seems to be available and it contains the block name (not exactly but can be used for the same check). However, the className stopped working in newer Gutenberg versions so it needs further confirmation.
Reference to where className was previously used but removed: https://github.com/Automattic/jetpack/pull/15906
marekhrabe
on 22 Jun 2020
Using className checks instead of useBlockEditContext will likely fix the incompatibility with both WP 5.3 and 5.4. However, that's pure core. If you also have Gutenberg 8.1 and newer, className checks are no longer possible. However, Gutenberg 8.1 already has useBlockEditContext as seen in:
Next steps
We need to find all usages of useBlockEditContext in editor.MediaPlaceholder calls and implement a logic that checks both the className and useBlockEditContext, while having guards in place that in no situation we use useBlockEditContext without it being available.
If this way doesn't work, I'm afraid we need to remove those features or only make them available if we detect the hook is available, as there seems no other API capable of checking what we need.
marekhrabe
on 22 Jun 2020
Using
classNamechecks instead ofuseBlockEditContextwill likely fix the incompatibility with both WP 5.3 and 5.4. However, that's pure core. If you also have Gutenberg 8.1 and newer,classNamechecks are no longer possible. However, Gutenberg 8.1 already hasuseBlockEditContextas seen in:
I think we shouldn't rely on this the className attribute at all. It makes the implementation unstable.
Next steps
We need to find all usages of
useBlockEditContextineditor.MediaPlaceholdercalls and implement a logic that checks both theclassNameanduseBlockEditContext, while having guards in place that in no situation we useuseBlockEditContextwithout it being available.
We should consider getting rid of the hook too. Guessing it's possible using different filters. Trying it on https://github.com/Automattic/jetpack/pull/16171. This PR has many changes to respect what is the primary branch.
If this way doesn't work, I'm afraid we need to remove those features or only make them available if we detect the hook is available, as there seems no other API capable of checking what we need.
🤞
retrofox
on 22 Jun 2020
I think using className would be a valid fallback where the hook is not available. I consider it stable enough because className will only be used with old WP/Gutenberg versions and the code of those is known and will never update.
If you find a completely different way, that would be the best, otherwise I think we should use className or completely disable those features when we know the hook is not available
marekhrabe
on 22 Jun 2020
yes, fair enough. Not sure if it works. Going to spend some minutes on this.
retrofox
on 22 Jun 2020
I think we should use className or completely disable those features when we know the hook is not available
I'm leaning towards disabling the notice when useBlockEditContext is not available. The feature works only for simple sites anyway, where useBlockEditContext is available, so disabling it for older WP versions should not change anything.
@jeherve How would you feel about that?
obenland
on 22 Jun 2020
The reason useBlockEditContext doesn't break the VideoPress block is that its use sits behind an isSimpleSite check, while for the cover block it gets called before that.
obenland
on 22 Jun 2020
I implemented a solution removing the useBlockEditContext using a different filter to extend the EditoBlock function of the cover block. Basically, it picks up the block name and passes it to the HOC to finally provide this value to the media cover context.
Picking up the name...
export default ( settings, name ) => {
if ( ! isUpgradable( name ) ) {
return settings;
}
return {
...settings,
edit: JetpackCoverBlockEdit( name )( settings.edit ),
};
};
... passing to the HOC
const JetpackCoverBlockEdit = ( blockName ) => createHigherOrderComponent(
( CoverBlockEdit ) => props => {
// ...
return (
// ...
);
},
'JetpackCoverBlockEdit'
)
Providing block name:
return (
<Fragment>
<CoverMediaProvider
onFilesUpload={ handleFilesPreUpload }
blockName={ blockName }
>
// ...
The whole implementation is https://github.com/Automattic/jetpack/pull/16240.
Also, @obenland did another approach to tackle the same issue. It adds the filter only if the cover block _is upgradable_. Here the PR https://github.com/Automattic/jetpack/pull/16241.
Both branches of these PRs have been branched off from update/cover-get-allowed-mime-types https://github.com/Automattic/jetpack/pull/16171
retrofox
on 23 Jun 2020
I would personally vote for the upgradable check approach, for a few reasons:
- It feels a bit simpler and quicker to parse and understand for someone new to the codebase.
- Since WordPress 5.5 will be released this summer, and we'll then have access to
useBlockEditContextsince we won't support WP 5.3 anymore, I feel comfortable going with that approach. - If in the future we decide to start using such nudges outside of simple sites, by then we'll most likely already have access to
useBlockEditContextbecause of the above.
What do y'all think?
jeherve
on 24 Jun 2020
@jeherve I agree. I've voiced something similar elsewhere https://github.com/Automattic/jetpack/pull/16241#issuecomment-648712233
marekhrabe
on 24 Jun 2020
Related issues
jeherve
·
3Comments
ockham
·
3Comments
beaulebens
·
3Comments
Viper007Bond
·
3Comments
crunnells
·
3Comments
Most helpful comment
The reason
useBlockEditContextdoesn't break the VideoPress block is that its use sits behind anisSimpleSitecheck, while for the cover block it gets called before that.