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.

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

Prevent usage of gutenberg_url in block-library #58242

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

luisherranz
Copy link
Member

What?

Prevent the usage of gutenberg_url in the interactive blocks of the block-library package.

Why?

Because we mistakenly left it in the #57778 PR and block-library should never contain direct usage of Gutenberg functions because it's synced "as it is" in WordPress Core, where those functions don't exist.

How?

By checking first if Gutenberg is activated, and if it's not, using includes_url instead of gutenberg_url.

@luisherranz luisherranz added [Type] Bug An existing feature does not function as intended Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) labels Jan 25, 2024
@luisherranz luisherranz self-assigned this Jan 25, 2024
'@wordpress/block-library/file-block',
gutenberg_url( '/build/interactivity/file.min.js' ),
'@wordpress/block-library/file',
defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN ? gutenberg_url( '/build/interactivity/file.min.js' ) : includes_url( 'blocks/file/view.min.js' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking my understanding here. What's the long term vision for these wp_register_script_module calls? Are they going to be done automatically using block.json or if we need conditionals, maybe instead we should have generic calls like wp_hydrate_block( 'core/file' ) or something where the actual files are not hard-coded by guessed from block.json or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I remember @sirreal is working on a viewModule and viewScriptModule feature in block json. So should be done automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. In the short term, we should replace them with viewScriptModule entries in the block.json. Then, at some point, I'd like to resume our work on behaviors where a single block can have a set of behaviors defined, and depending on the active ones, WordPress will enqueue the required JS and CSS files for those behaviors.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not viewModule? or do we use that for something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

or viewScript?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the beginning, we called it the Modules API but then everything was renamed to "script modules":

Copy link
Member

Choose a reason for hiding this comment

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

Here's the viewScriptModule for Core: WordPress/wordpress-develop#5860

viewModule support was implemented in Gutenberg, but when "modules" became "script modules" in Core it seemed to make sense to align and use viewScriptModule, but that's still up for review.

Copy link
Contributor

@cbravobernal cbravobernal left a comment

Choose a reason for hiding this comment

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

LGTM

@cbravobernal cbravobernal enabled auto-merge (squash) January 25, 2024 11:40
@cbravobernal cbravobernal disabled auto-merge January 25, 2024 11:40
@cbravobernal cbravobernal merged commit 1ea6c22 into trunk Jan 25, 2024
55 checks passed
@cbravobernal cbravobernal deleted the fix-gutenberg-url-in-block-library branch January 25, 2024 11:55
@github-actions github-actions bot added this to the Gutenberg 17.7 milestone Jan 25, 2024
cbravobernal pushed a commit that referenced this pull request Jan 25, 2024
* Prevent usage of gutenberg_url in block-library

* Add includes_url

* Update enqueue module identifiers
@cbravobernal
Copy link
Contributor

I just cherry-picked this PR to the release/17.6 branch to get it included in the next release: 385bf9f

@cbravobernal cbravobernal removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Jan 25, 2024
cbravobernal pushed a commit that referenced this pull request Jan 25, 2024
* Prevent usage of gutenberg_url in block-library

* Add includes_url

* Update enqueue module identifiers
@MaggieCabrera MaggieCabrera added [Feature] Blocks Overall functionality of blocks [Package] Block library /packages/block-library labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants