Create frontend entry points for block-library, outputting code loadable from the browser. #30341
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
vcanales
added
[Type] Build Tooling
7 tasks
|
Size Change: 0 B Total Size: 1.42 MB ℹ️ View Unchanged
|
vcanales
force-pushed
the
build-block-frontend-files
branch
from
e0831a4 to
2a38f1e
Compare
vcanales
force-pushed
the
build-block-frontend-files
branch
from
2a38f1e to
7c3da62
Compare
|
For adding basic testing instructions, do we expect |
That's correct; there should be no change between running that command in this PR vs trunk. In order to check this, I ➜ gutenberg git:(build-block-frontend-files) shasum -a 256 build/
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 build/➜ trunk git:(trunk) shasum -a 256 build/
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 build/(different directories as well, to avoid accidentally hashing the same thing twice) Other than running that check independently, and checking that e2e tests run as expected after building this branch, I can't think of other tests that can be done; AFAIK we don't have a way to test the result of builds at the moment. I'm open to suggestions for further testing, please let me know if you think of something! |
gwwar
approved these changes
Apr 6, 2021
There was a problem hiding this comment.
Giving tentative approval here. I do think we'll want a +1 from folks who have helped maintain the webpack config.
Did a bit of smoke testing with npm run build and I also compared plugin output from run build:plugin-zip using 7c3da62 and then comparing with 4b8b2d9 (a commit before changes here).
| Only difference in output is artifact build version |
|---|
 |
youknowriad
reviewed
Apr 7, 2021
gziolo
reviewed
Apr 8, 2021
| const scriptPaths = fastGlob.sync( | ||
| './packages/block-library/src/**/frontend.js', | ||
| { | ||
| ignore: [ '**/build*/**' ], |
There was a problem hiding this comment.
Why was it necessary? There are no build folders inside the block-library/src/ folder or do I miss something?
There was a problem hiding this comment.
It was necessary at some point, when instead of going into /src, I would start from block-library/, removing in #30629
gziolo
reviewed
Apr 8, 2021
| ); | ||
|
|
||
| const scriptEntries = scriptPaths.reduce( ( entries, scriptPath ) => { | ||
| const [ blockName ] = scriptPath.match( blockNameRegex ) || []; |
There was a problem hiding this comment.
What will happen when the block name is undefined?
There was a problem hiding this comment.
fast-glob will look for frontend.js files is within block source directories, so the value of scriptPaths will always be either an empty array, or a list of paths in the form './packages/block-library/src/<blockName>/frontend.js', in which case /(?<=src\/).*(?=(\/frontend))/g would always have a match.
Because of this, the guard I set there is unnecessary.
In order to make sure, I ran npm run build, without the guard present, under the following conditions:
- no
frontend.jsfiles present. frontend.jsfile present in a subdirectory of a block's source directory, in which case the file gets built into that same subdirectory in the block's build directoryfrontend.jspresent in multiple block directories.
I think the best think to do here would be deleting the misleading guard.
I've done this and also added more comments going through the logic in #30629
There was a problem hiding this comment.
Got it, makes total sense, this [] was confusing 😄
gziolo
reviewed
Apr 8, 2021
| * we build it in the block's directory. | ||
| */ | ||
| if ( rawRequest && rawRequest.includes( '/frontend.js' ) ) { | ||
| return `./build/block-library/blocks/[name]/frontend.js`; |
There was a problem hiding this comment.
That's nice, it's going to be located next to block.json in the build folder so it should be a simple reference once we introduce frontend field in block.json. I like it 👍🏻
|
Nice work on this PR. Thank you for exploring this new capability. I'm looking forward to seeing how it works in action. I'd like to confirm first it's solid enough before we jump into extending I also left two questions about implementation details that would be great to clarify. |
6 tasks
pento
reviewed
Apr 16, 2021
|
|
||
| return { | ||
| ...entries, | ||
| [ blockName ]: scriptPath, |
There was a problem hiding this comment.
Using the blockName as the key may cause interesting issues in the future, if a block has the same name as a package, since packageEntries and scriptEntries are merged below, any duplicate named entries in scriptEntries will override the entry in packageEntries.
There was a problem hiding this comment.
Good catch. At the moment we have a single script entry so it isn't an issue but with the number of packages and blocks that exist, it's possible that they could use the same name. It should be further investigated.
gziolo
reviewed
Apr 16, 2021
| path: __dirname, | ||
| library: [ 'wp', '[name]' ], | ||
| library: [ 'wp', '[camelName]' ], |
There was a problem hiding this comment.
One more thing that needs more thinking. I don't think we want to produce wp globals for frontend scripts. Taking into account the comment from @pento https://github.com/WordPress/gutenberg/pull/30341/files#r614557004, it might be worth exploring whether to use two independent webpack configs to gain more flexibility in how those two different use cases are handled. We maintain multiple (2) webpack configs in WordPress core:
https://github.com/WordPress/wordpress-develop/blob/40728234568b07d6bcaa456816bf284f8c5cc670/webpack.config.js#L13-L16
11 tasks
annezazu
added
the
[Focus] Accessibility (a11y)
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.
Description
Replaces #29537.
In order to achieve accessibility for a mobile responsive navigation block, it is necessary to create additional JavaScript to be loaded from the frontend. In order to access this code on demand — only when the block consuming it is rendered — it has to be built into a directory accessible through a URL.
This PR has
webpackfind files namesfrontend.jswithin@wordpress/block-library, and builds them into the respective block's build directory. eg. The Navigation block has afrontend.js(packages/block-library/src/navigation/frontend.js) file which is going to be built intobuild/block-library/blocks/navigation/frontend.js.How has this been tested?
We don't currently have tools that test the build process directly, but #30047 includes the code introduced by this branch, applies one of the loading methods described in the README file, and has an e2e test verifying that the loading method works as intended.