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

Documentation: Update documentation about build process changes #66428

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Oct 24, 2024

What?

Update documentation about build process.

#65064 updated the way that script modules from packages are bundled for Gutenberg and WordPress.
#66272 updated the way that scripts from packages are bundled for Gutenberg and WordPress.

Why?

The build system has been modified and requirements have changed, it's important to keep documentation up to date. The system is expected to be stable now.

Testing Instructions

Do the docs appear accurate and make sense?

Document the wpScript and wpScriptModuleExports fields and their
purposes.
The block-library includes instructions for setting up a new block,
document special steps necessary for script modules.
@sirreal sirreal added the [Type] Developer Documentation Documentation for developers label Oct 24, 2024
Copy link

github-actions bot commented Oct 24, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: justintadlock <greenshady@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

packages/README.md Outdated Show resolved Hide resolved
packages/README.md Outdated Show resolved Hide resolved
packages/README.md Outdated Show resolved Hide resolved
// Include this line to include the package as a WordPress script.
"wpScript": true,
// Include this line to include the package as a WordPress script module.
"wpScriptModuleExports": "./build-module/index.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I like this. Why should a package.json mention a built file. why wpModule: true is not enough? since we already have module defined in the package.json ?

Copy link
Member

Choose a reason for hiding this comment

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

I think one aspect needs to be documented. Using wpScript or/and wpScriptModuleExports marks the package as production package for strict verification of compatibility with GPL license.

module isn't configured correctly at the moment. Eventually, we can explore migrating to "type": "module" and exports configuration but it's a separate effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

module isn't configured correctly at the moment.

I'm not sure I understand, we've been relying on module to output ESmodules forever, why is it not correct?

Copy link
Contributor

@youknowriad youknowriad Oct 25, 2024

Choose a reason for hiding this comment

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

I was looking at https://github.com/WordPress/gutenberg/blob/trunk/packages/a11y/package.json for instance

That package has both "module" and "wpScriptModuleExports" and they point to different modules/APIs. So we're saying that this package ships "two different ESmodules" in one package. As an npm package consumer, I'm really confused what import something from '@wordpress/a11y' will yield to me. Which one of these is going to be used? Ultimately a package can ship multiple modules, that's ok but not two versions of the same module.

Copy link
Member

Choose a reason for hiding this comment

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

I think one aspect needs to be documented. Using wpScript or/and wpScriptModuleExports marks the package as production package for strict verification of compatibility with GPL license.

I opened a follow-up #66562.


For packages that should ship as a WordPress script, include `wpScript: true` in the `package.json` file. This tells the build system to bundle the package for use as a WordPress script.

For packages that should ship as a WordPress script module, include a `wpScriptModuleExports` field the `package.json` file. The value of this field can be a string to expose a single script module, or an object with a [shape like the standard `exports` object](https://nodejs.org/docs/latest-v20.x/api/packages.html#subpath-exports) to expose multiple script modules from a single package:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a duplication of the exports field for regular npm packages. Do we really need to invent our own convention?

Copy link
Member

Choose a reason for hiding this comment

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

@sirreal can provide a more exhaustive response. We were discussing using the official configuration for ES Modules:

It'll eventually happen, but more work is necessary. For example, all import statements that contain file paths must contain the file extension, for example: import './file.js';. That isn't the case currently.

@gziolo
Copy link
Member

gziolo commented Oct 25, 2024

@sirreal, I refined the section about the view scripts for blocks in 03fb0aa. It's only view script modules at the moment, so the guidelines included were outdated.


```php
function render_block_core_blinking_paragraph( $attributes, $content ) {
$should_load_view_script = ! empty( $attributes['isInteractive'] ) && ! wp_script_is( 'wp-block-blinking-paragraph-view' );
Copy link
Member

Choose a reason for hiding this comment

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

Noting that there is no wp_script_module_is_enqueued helper method. It isn't strictly necessary here, but it might be helpful for extenders in different scenarios.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thank you for opening this PR, @sirreal. It documents the current approach for annotating package.json files to instruct webpack bundler of how to create entry points for scripts and script modules.

It would also be helpful to explain how the production packages are detected when performing license checks as noted in https://github.com/WordPress/gutenberg/pull/66428/files#r1816140475.

I also appreciate the questions raised by @youknowriad. We should continue discussing how to evolve the configuration entries to apply these changes in future iterations.

Copy link

Flaky tests detected in 03fb0aa.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11514286825
📝 Reported issues:

@sirreal
Copy link
Member Author

sirreal commented Oct 25, 2024

This reflects the current state of things so I'm going to land it. The conversation is largely around the changes to the build that are already live and not about the documentation. I'll continue to reply to the conversation after merging.


It would also be helpful to explain how the production packages are detected when performing license checks as noted in https://github.com/WordPress/gutenberg/pull/66428/files#r1816140475.

Noted, I'll address this in a follow-up. I want to prioritize getting the essentials documented.

@sirreal sirreal enabled auto-merge (squash) October 25, 2024 08:39
@sirreal sirreal merged commit 7dbe0be into trunk Oct 25, 2024
62 checks passed
@sirreal sirreal deleted the update/document-updated-build-process branch October 25, 2024 09:12
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 25, 2024
@sirreal
Copy link
Member Author

sirreal commented Oct 25, 2024

I'll try to answer all of the questions and provide some motivation. This is all very complex. What a "package" is can be very different things in different contexts.

The motivation here is actually to simplify the build so that it can be understood through some declarative configuration instead of having 2 imperative builds that must be kept in sync across Gutenberg and WordPress Core, while improving support for Script Modules and other things like npm workspaces.


Packages:

  • Most are published to npm and may be consumed by third parties. These may be used in bundlers or in a Node.js environment. They may also be installed but not actually used directly by third parties, but instead via the WordPress environment as scripts or script modules.
  • Some are only published to and consumed from npm. Theses are typically development packages.
  • Some packages are bundled into WordPress as scripts, which requires special logic. Previously was indicated by the @wordpress/ package name prefix and including it as a production dependency of Gutenberg (wordpress-develop still does it this way but I'd like to update that). There were also some hard-coded exceptions.
  • Some packages are bundled into WordPress as script modules. Previously that was just hard-coded to some Interactivity API and block-library view script modules.
  • Some packages (only a11y at this time) are included in WordPress as both scripts and script modules.
  • Scripts and packages have always had a 1:1 relationship - 1 package may have 1 script.
  • Many third party consumers install the package, but actually use it via a WordPress environment.

To complicate things, package.json can define the package with different type (Gutenberg does not do that anywhere at this time). commonjs means the package should use Node.js require, and module means it should be treated as an ES Module. This is an indicator for Node.js. Other tooling like bundlers will respect this in different ways.

The module field is a pseudo-standard that many bundlers recognize to point to the main package entrypoint using ES Modules syntax. Many bundlers use this field and then transform import/export syntax into some other form and then run as scripts. This turns out to be a little bit broken because ES Modules syntax is not just import/export, but also things like import.meta. This was a problem recently: #64845 (comment)


A lot of these recent changes move logic that was information about package but was centralized in the build system and duplicated in Gutenberg and Core. The changes make the packages declare if and how they should be bundled for WordPress.

Nothing has changed for npm packages. Nothing has changed for scripts. Nothing has changed for script modules. This is all about making the build support scripts and script modules and rely on declarative configuration.


[Regarding wpScriptModuleExports,] why should a package.json mention a built file.

This is already a common practice, main and module fields both point to built files, as would an exports field. These fields indicate the entrypoints a package exposes. In the case of exports (or wpScriptModuleExports) this can include exposed subpath exports.

Why wpModule: true is not enough?

wpModule is insufficient because it does not support subpath exports. This allows a package to declaratively expose. wpScriptModuleExports is declarative information for the build system, it describes the entrypoints that are exposed as WordPress script modules.

Individual packages may expose several script modules, block-library is a good example of that.

wpScript: true is sufficient because scripts have always had a one-to-one relationship with packages.

I was looking at [a11y] for instance … [it] has both "module" and "wpScriptModuleExports" and they point to different modules/APIs.

module is a quasi-standard that many common bundlers use by default. It points to a main entrypoint for the package that uses import/export syntax. But this field is really only for bundlers, it does not define the package as an ES Module package, that's handled by type: module.

The nonstandard wp*fields that have been added are specifically for bundling for WordPress, they're not suitable or recognized by any tooling other than our own.

With a11y, the only package that currently exposes a script and a script module (and a package on npm), the API is the same.

So we're saying that this package ships "two different ESmodules" in one package.

Not really. wpScriptModuleExports isn't for third parties, it's for our build system. It does describe what is available as a WordPress script module.

I'm really confused what import something from '@wordpress/a11y' will yield to me. Which one of these is going to be used?

The same thing as before… it depends 🙂

  • Consumed and bundled via npm: the exact same thing.
  • Used by extenders with wp-scripts / DEWP in a script: the same thing - the wp-a11y script.
  • Used by extenders with wp-scripts / DEWP in a script module: The @wordpress/a11y script module with the same API.

Isn't this a duplication of the exports field for regular npm packages. Do we really need to invent our own convention?

Conceptually it's similar as the exports field (although it's simpler, for example conditional exports are not supported), but this is declarative for WordPress script module bundling. I think re-using the convention is helpful, but switching to use exports would be a breaking change and could be disruptive depending on how consumers are using the package.

Again, the goal here is to support our needs —declare the exposed script modules for bundling for WordPress— without disrupting anything else.

@youknowriad
Copy link
Contributor

The changes make the packages declare if and how they should be bundled for WordPress.

Just want to share that I totally agree with normalizing all this config in package.json, it's way better than having all these scattered configs in webpack and else. Now I would like to talk about the "how" and the "details".

module is a quasi-standard that many common bundlers use by default. It points to a main entrypoint for the package that uses import/export syntax. But this field is really only for bundlers, it does not define the package as an ES Module package, that's handled by type: module.

The problem is that our packages are both "modules" and "scripts", so how do you do that today in npm world?

The nonstandard wp*fields that have been added are specifically for bundling for WordPress, they're not suitable or recognized by any tooling other than our own.

Sure but for me, we're risking of creating two APIs for the same module by pointing "module" to a file and "wpScriptModuleExports" to another. I'd rather not do that and having an npm package ship one or multiple module but consistently. Most npm packages ship one module, the make it available as CJS or ESM but the API is the same. But for a11Y it's not really the case. We have 1 CJS version that is equivalent to another ESM version but we also have another slightly different ESM version.

Not really. wpScriptModuleExports isn't for third parties, it's for our build system. It does describe what is available as a WordPress script module.

This is where I'm not following, and this is the main important point for me. we need WordPress to ship ES modules, why can't WordPress just use the "module" version of our packages in that case?

If the answer is that the current format for our module exports is broken, that's ok, we should fix it. I want to avoid having WordPress ship a module with a given API but the actual npm package is a different API. I think that's not desirable and will create confusion for documentation...

@sirreal
Copy link
Member Author

sirreal commented Oct 25, 2024

The problem is that our packages are both "modules" and "scripts", so how do you do that today in npm world?

The modern way of handling that would be to use conditional exports. That's not unlike the approach that's been taken recently with the build for WordPress.

Sure but for me, we're risking of creating two APIs for the same module by pointing "module" to a file and "wpScriptModuleExports" to another.

This has always been the case. There's always a risk. The standard exports field has the exact same problem with conditional exports and the dual-package hazard. It may be more apparent now, but the package shipped to npm and the software bundled in WordPress have always been different.

Most npm packages ship one module, the make it available as CJS or ESM but the API is the same.

This is less true with exports and subpaths, packages are gradually exposing more entrypoints just like has been done with block-library:

"wpScriptModuleExports": {
"./file/view": "./build-module/file/view.js",
"./image/view": "./build-module/image/view.js",
"./navigation/view": "./build-module/navigation/view.js",
"./query/view": "./build-module/query/view.js",
"./search/view": "./build-module/search/view.js"
},

But for a11Y it's not really the case. We have 1 CJS version that is equivalent to another ESM version but we also have another slightly different ESM version.

I understand your position and agree that we want to be consistent. I don't think this is that different from the previous situation. The things that are published as packages to npm are not the same things that are shipped with WordPress. Building and bundling them fundamentally changes them.

Scripts and Script Modules have some fundamental differences. Moreso in WordPress where the same APIs cannot be used for initialization (scripts use inline scripts with imperative initialization, modules use the data passing API and should perform initialization themselves).

For a11y, we decided it was possible to expose the same API as a script and a script module. #65101 extracted a shared "core" implementation, then exposes a script and a script module version that hides their necessary differences.

For some existing scripts, this may work well. For others it may not and a completely separate module may need to be introduced. But when the API can remain the same.

This is where I'm not following, and this is the main important point for me. we need WordPress to ship ES modules, why can't WordPress just use the "module" version of our packages in that case?

One fundamental difference is that scripts and script modules cannot interdepend (at this time). Script modules cannot depend on scripts and I don't see that changing. It's not as simple as taking a module "build" (which has had some simple babel transformations applied) and compiling it into a WordPress script module will not often not work because the dependencies are not satisfied. For a11y the code using wp-i18n had to be removed from the WordPress module version in order for it to work correctly.


The behavior of these things in WordPress is different. It's not the same as when they're bundled with other applications or used elsewhere. This is true for both scripts and script modules and it's always been the case.

The approach taken for script modules recognizes that the build and environment for WordPress is different. It has different constraints and possibilities.


I will add another difficulty that does concern me and I've. Right now scripts import @wordpress/a11y even though it really means to use wp-a11y script, or window.wp.a11y at runtime. As we consider allowing scripts to depend on script modules and use them via import(…), some of the module identifiers become ambiguous.

If a script does import {speak} from '@wordpress/a11y' or const { speak } = import('@wordpress/a11y') what is it importing? How does the build system know what to use and whether it's available?

I've been looking at import attributes as a possible solution for this problem when it arises:

const { speak } = import('@wordpress/a11y', { with: { wpModule: true } })

@cbravobernal cbravobernal changed the title Documenation: Update documentation about build process changes Documentation: Update documentation about build process changes Oct 25, 2024
@gziolo
Copy link
Member

gziolo commented Oct 28, 2024

This is where I'm not following, and this is the main important point for me. we need WordPress to ship ES modules, why can't WordPress just use the "module" version of our packages in that case?

That's an interesting discussion point. Most of the existing WordPress packages aren't fully compatible with ES Modules despite the fact that we publish to npm build-module folder listed as the target for the module field in package.json file. @sirreal, already covered it, but module is only a way to tell bundlers that there are import and export statements so you can use that to perform tree-shaking efficiently, but it's still up to the bundler to decide what the output for the chunk will be. I think it doesn't matter that much how the packages get published to npm, but rather the focus should be on how the package is structured so it can be consumed as ES Module after bundling. As of today, it turns out it isn't that simple because of several architectural decisions made in the past. The primary challenge is how the APIs in the individual packages are configured and how the dependencies get loaded on demand asynchronously.

To you point, eventually, we should be in the place, when both script module and script use the same entry point, but there still is going to be some backward compatibility layer where the logic specific only for scripts would have to be kept around, for example: functions calls to configure @wordpress/api-fetch. The other aspect that we considered when exposing @wordpress/a11y as a script module in WordPress was ensuring that the bundle shipped to the user is optimized for size as it is consumed on the front end. That's why it doesn't depend on @wordpress/i18n when bundled as a script module. Plus, it doesn't have to depend on @wordpress/dom-ready because script modules are deferred by design. There are multiple factors to take into account when providing support for these two models for handling JavaScript resources.

karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…ress#66428)

Document recent changes to the build system that change are requirements
for publishing WordPress scripts and script modules.

WordPress#65064 updated the way that script modules from packages are bundled for Gutenberg and WordPress.
WordPress#66272 updated the way that scripts from packages are bundled for Gutenberg and WordPress.

---------

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: justintadlock <greenshady@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants