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

Scripts: Add new flag to allow customization of the src directory. #39618

Merged

Conversation

ryanwelcher
Copy link
Contributor

What?

Adds a new CLI flag to allow customization of the "src" directory called --webpack-src-dir.

Why?

The scripts package allows for entry point customization when being called i.e wp-scripts start ./custom-dir however those custom entry points will not benefit from the auto-detection of block.json files and the ability to copy PHP files due to "src" being hardcoded in the code that provides those features.

In some cases, being able to change the name of the directory helps to clarify its usage, especially if there are other directories that also contain source code. This is seen in WordPress/gutenberg-examples#195 (comment), where we are needing to have directories that handle blocks and add a separate one for non-block code examples. See WordPress/gutenberg-examples#195 (comment) for more context on why the src directory doesn't work for all examples.

How?

If the new flag is detected, the passed directory is used, otherwise "src" is used.

Testing Instructions

  • I created a block using npx @wordpress/create-block@ scripts-test
  • Renamed src to test-directory
  • Ran node {FULLPATH}/gutenberg/packages/scripts/bin/wp-scripts.js -- start --webpack-src-dir=test-directory

Update error messaging and inline comments.
@ryanwelcher ryanwelcher added [Tool] WP Scripts /packages/scripts Developer Experience Ideas about improving block and theme developer experience labels Mar 21, 2022
@ryanwelcher ryanwelcher marked this pull request as ready for review March 21, 2022 20:08
@ryanwelcher ryanwelcher requested review from mkaz and gziolo and removed request for gziolo, ntwb, ajitbohra and nerrad March 21, 2022 20:08
if ( ! hasProjectFile( 'src' ) ) {
// Continue only if the source directory exists.
if ( ! hasProjectFile( process.env.WP_SRC_DIRECTORY ) ) {
log(
Copy link
Member

Choose a reason for hiding this comment

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

I remember some folks reporting a similar message on the terminal was confusing in case they using a custom config that overrides the entry points.

Copy link
Member

Choose a reason for hiding this comment

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

It made me think that we could use a dynamic entry as documented in https://webpack.js.org/configuration/entry-context/#dynamic-entry. We would never run this code when the project overrides entry points.

If a function is passed, then it will be invoked on every make event.

Note that the making event triggers when webpack starts and for every invalidation when watching for file changes.

In fact, this would solve a more pressing issue we recently discovered with @schmitzoide when working on a plugin with multiple blocks. Currently, the logic for finding entry points runs only once in the watch mode. Therefore you need to restart the development build whenever new blocks get introduced or scripts for the block change. Using a callback function should resolve the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Related comment I mentioned: #38739 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

@gziolo @ryanwelcher Hope this PR resolves the confusing behaviour for #38739 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I added commit 37ec581 to see if that resolves the issue.

Copy link
Member

Choose a reason for hiding this comment

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

I can confirm that the message isn't shown when we use the default handling for entry points.

I can also confirm that entry points get dynamically updated in the watch mode when block.json changes.

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.

This is a great idea! Thank you for starting this PR. I left my feedback, and I believe this is nearly ready to go. I want to test it extensively when you get the chance to respond to the idea of using dynamic entry points. It’s probably as simple as not calling the existing function 😃

It would be great to update https://github.com/WordPress/gutenberg/blob/trunk/packages/scripts/README.md and explain how to pass a custom source folder. Ideally, we move that to its section and reference from both start and build commands.

@gziolo gziolo added [Type] Feature New feature to highlight in changelogs. [Type] New API New API to be used by plugin developers or package users. and removed [Type] Feature New feature to highlight in changelogs. labels Mar 28, 2022
@gziolo gziolo changed the title [Scripts]: Add new flag to allow customization of the src directory. Scripts: Add new flag to allow customization of the src directory. Mar 28, 2022
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.

We still need to improve the documentation for @wordpress/scripts to educate how to use the new --webpack-src-dir CLI flag. In general, we need better docs for how to work with the build process that has become very flexible but also a bit too magical 😄

@gziolo
Copy link
Member

gziolo commented Mar 28, 2022

I see I messed up something with whitespaces when editing with GitHub UI:

Screenshot 2022-03-28 at 15 32 14

It's always tricky when trying to update a forked repository.

@@ -232,7 +232,7 @@ const config = {
new CopyWebpackPlugin( {
patterns: [
{
from: copyWebPackPattens,
from: copyWebPackPatterns,
Copy link
Member

Choose a reason for hiding this comment

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

NIt: it's webpack not WebPack 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Nice catch - I've updated the reference.

@ryanwelcher
Copy link
Contributor Author

We still need to improve the documentation for @wordpress/scripts to educate how to use the new --webpack-src-dir CLI flag. In general, we need better docs for how to work with the build process that has become very flexible but also a bit too magical 😄

💯 on this for sure. I have added some updated information with this PR but I think we may need to restructure the docs as it's broken down by command.

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.

Everything looks great in regards to the new CLI option introduced.

I have added some updated information with this PR but I think we may need to restructure the docs as it's broken down by command.

Yes, totally agree. I think we still need to have all commands listed in the README file but we definitely should make that shorter and reference more detailed documents/guides grouped by concepts. In particular, it would be important to have something that covers all related concepts to the build process in one place rather than duplicated for build and start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Tool] WP Scripts /packages/scripts [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants