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

Modules API: Refactor, tests, and final dependencies array structure #57231

Merged
merged 11 commits into from Dec 20, 2023

Conversation

luisherranz
Copy link
Member

@luisherranz luisherranz commented Dec 19, 2023

What?

This is a general refactor of the code, including more test coverage and a few improvements on the API.

Starting from this PR, the dependencies array has the following structure:

An array of module identifiers of the dependencies of this module. The dependencies can be strings or arrays. If they are arrays, they need an id key with the module identifier, and can contain a type key with either static or dynamic. By default, dependencies that don't contain a type are considered static.

Examples:

// foo.js
import x from "static-dependency";
import y from "another-static-dependency";
import z from "yet-another-static-dependency";

const d = await import("dynamic-dependency");
gutenberg_register_module( 'foo', '/foo.js', array(
  'static-dependency',
  array(
    'id' => 'another-static-dependency'
  ),
  array(
    'id'   => 'yet-another-static-dependency',
    'type' => 'static'
  ),
  array(
    'id'   => 'dynamic-dependency',
    'type' => 'dynamic'
  ),
) );

This opens the door for future additions, like for example dependency versions to support import map scopes:

gutenberg_register_module( 'lib', '/lib.v1.js', array(), '1.2.3' );
gutenberg_register_module( 'lib', '/lib.v2.js', array(), '2.3.4' );

gutenberg_register_module( 'foo', '/foo.js', array( 'lib' ) );
gutenberg_register_module( 'bar', '/bar.js', array( array( 'id' => 'lib', 'version' => '^1.0.0' ) ) );
<script type="importmap">
  {
    "imports": {
      "lib": "/lib.v2.js"
    },
    "scopes": {
      "/bar.js": {
        "lib": "/lib.v1.js"
      }
    }
  }
</script>

Why?

Apart from the general improvements here in Gutenberg, I want to open a PR on WordPress Core as soon as this is merged, so more folks can test it out and give feedback.

How?

I reviewed all the code, comments and tests.

Testing Instructions

Either add some interactive blocks to your page and check that the modules, preloads and import maps are added correctly to the page, or use the gutenberg_register_module and gutenberg_enqueue_module functions in your own code.

@luisherranz luisherranz added [Type] Enhancement A suggestion for improvement. [Feature] Script Modules API Related to the Script Modules API that adds support for native ES modules and import maps labels Dec 19, 2023
@luisherranz luisherranz self-assigned this Dec 19, 2023
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/modules/class-gutenberg-modules.php
❔ phpunit/experimental/modules/class-gutenberg-modules-test.php

Copy link
Contributor

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Sorry I can't give a better review; there's a lot in here which I'm not too familiar with. Left some random thoughts though.

lib/experimental/modules/class-gutenberg-modules.php Outdated Show resolved Hide resolved
lib/experimental/modules/class-gutenberg-modules.php Outdated Show resolved Hide resolved
lib/experimental/modules/class-gutenberg-modules.php Outdated Show resolved Hide resolved
lib/experimental/modules/class-gutenberg-modules.php Outdated Show resolved Hide resolved
lib/experimental/modules/class-gutenberg-modules.php Outdated Show resolved Hide resolved
@luisherranz
Copy link
Member Author

Thanks, Dennis. I've address all your feedback 🙂

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.

Tested and working as expected.
Apart from the nitpicks discussed, I could not find anything else to mention. It will still need a backport that will be re-reviewed by core committers.

@luisherranz
Copy link
Member Author

Thanks, Carlos!

Let's merge this now and I'll start working on the WP Core backport.

@luisherranz luisherranz merged commit 79cca7e into trunk Dec 20, 2023
51 checks passed
@luisherranz luisherranz deleted the modules-api-change-dependencies-array-structure branch December 20, 2023 14:58
@github-actions github-actions bot added this to the Gutenberg 17.4 milestone Dec 20, 2023
@bph bph added the [Type] Code Quality Issues or PRs that relate to code quality label Dec 27, 2023
Copy link

github-actions bot commented Dec 27, 2023

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Type] Enhancement, [Type] Code Quality, [Feature] Modules API, Backported to WP Core.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Jan 15, 2024
@luisherranz luisherranz added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Script Modules API Related to the Script Modules API that adds support for native ES modules and import maps [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants