Prevent api-fetch and core-data from being imported in the block editor package #46302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
requested review from
draganescu,
talldan,
fluiddot and
ntsekouras
youknowriad
commented
Dec 5, 2022
| @@ -36,6 +36,159 @@ const typedFiles = glob( 'packages/*/package.json' ) | |||
| .filter( ( fileName ) => require( join( __dirname, fileName ) ).types ) | |||
| .map( ( fileName ) => fileName.replace( 'package.json', '**/*.js' ) ); | |||
|
|
|||
| const restrictedImports = [ | |||
There was a problem hiding this comment.
No change here, it was just extracted to be reused in the "override"
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
ntsekouras
approved these changes
Dec 5, 2022
youknowriad
commented
Feb 12, 2024
| @@ -1,6 +1,8 @@ | |||
| /** | |||
| * WordPress dependencies | |||
| */ | |||
| // Disable Reason: Needs to be refactored. | |||
| // eslint-disable-next-line no-restricted-imports | |||
There was a problem hiding this comment.
@draganescu No urgency here but it would be good to move this autocompleter elsewhere (editor package maybe).
youknowriad
commented
Feb 12, 2024
| @@ -1,6 +1,8 @@ | |||
| /** | |||
| * WordPress dependencies | |||
| */ | |||
| // Disable Reason: Needs to be refactored. | |||
| // eslint-disable-next-line no-restricted-imports | |||
| import apiFetch from '@wordpress/api-fetch'; | |||
There was a problem hiding this comment.
@talldan No urgency here but it would be good to find a solution for this. (move it to editor or something)
There was a problem hiding this comment.
@youknowriad I don't think this was anything to do with me, btw, but happy to have a think about it and help out where I can.
@ajlende originally implemented the feature, and then moved it to the block editor package, my role was only some refactoring back when it was still in the block-library package.
I don't think it can be moved to editor as there's multiple blocks using the component, and they can't import it from editor.
One thought is that maybe the API Fetch part can be moved out to the specific blocks, but might need some back compat, as plugins are using this (WooCommerce being the main one I can see). Not sure how easy that would be to do.
Or it could potentially be implemented in a similar way to LinkControl / __experimentalFetchLinkSuggestions where the fetch code is an editor setting, and then the component uses that. It doesn't necessarily guarantee back compat though, it won't cover if that component is used outside of a block editor.
Will keep thinking on it.
There was a problem hiding this comment.
Sorry for the wrong ping and yeah I agree that these are not always easy problems to tackle but I do believe important enough for the long term.
Shouldn't this component be using the mediaUpload setting passed to the BlockEditorProvider instead?
There was a problem hiding this comment.
Perhaps, at a first look it seems like mediaUpload / uploadMedia uses the /wp/v2/media endpoint and can upload multiple files, while this one uses /wp/v2/media/${ id }/edit endpoint and is just for modifying one file.
But I think that's a good call, maybe a new setting/function that works in a similar way to mediaUpload could be an option, or adapting mediaUpload so that it can handle edits.
There was a problem hiding this comment.
or maybe we can evolve the same function to work differently depending on whether you pass an id in the object or not.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What and why?
The block editor package is a package that is used to build third-party editors, potentially without WordPress, without any server (static page)... In that sense, any usage of "core-data" or "api-fetch" should be forbidden.
There are different potential approaches on how to integrate it with a server or WP. There's no magical solution, it all depends on the use-case. Some potential approaches we've used in the past:
BlockEditorProviderI've noticed a couple places where this rule has been breached and we'd need to refactor. But for now I'm adding an eslint rule to stop these shortcuts from proliferating and causing issues to third-party integrations.