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.
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
Create frontend entry points for block-library, outputting code loadable from the browser. #30341
Create frontend entry points for block-library, outputting code loadable from the browser. #30341
Changes from all commits
7c3da62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was it necessary? There are no
build
folders inside theblock-library/src/
folder or do I miss something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was necessary at some point, when instead of going into
/src
, I would start fromblock-library/
, removing in #30629There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen when the block name is undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast-glob
will look forfrontend.js
files is within block source directories, so the value ofscriptPaths
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:frontend.js
files present.frontend.js
file 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.js
present 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, makes total sense, this
[]
was confusing 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the
blockName
as the key may cause interesting issues in the future, if a block has the same name as a package, sincepackageEntries
andscriptEntries
are merged below, any duplicate named entries inscriptEntries
will override the entry inpackageEntries
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 introducefrontend
field inblock.json
. I like it 👍🏻There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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