Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#51966 new enhancement

npm/grunt watch/build task names are inconsistent and unintuitive

Reported by: iandunn's profile iandunn Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.1
Component: Build/Test Tools Keywords: has-patch 2nd-opinion
Focuses: javascript, docs Cc:

Description

#43055 / #44492 made a lot of changes to the typical dev workflow, and I often have trouble getting the right watch or build command going, because they don't seem intuitive or consistent to me.

The dev flag feels vague, and doesn't describe what it actually does (builds into src/ instead of build/), the npm commands aren't always internally consistent, or consistent w/ the corresponding grunt commands. They're also syntax differences between npm and grunt which need to be memorized.

Some examples:

  • There's npm run build and npm run build:dev, but the corresponding commands are npm run watch and npm run dev, instead of npm run watch:dev.
  • npm run build wraps grunt build, but npm run dev wraps grunt watch --dev; there is no corresponding grunt dev task.
  • running npm run watch --dev will run grunt watch (into build/) instead of grunt watch --dev (into src/). That's because npm uses -- -- to pass a flag to the proxied command, but grunt uses --.

The commands aren't clearly documented in the Handbook or readme file, so I have to read the package.json or Gruntfile.js, and try to memorize the inconsistencies.

Ideas

I think it'd help to:

  • Add clear documentation about the purpose of the src/ and build/ folders, the pros/cons of running from each, and the commands needed for each scenario.
  • Rename the commands/flags to be self-documenting and consistent.

I'll open a PR with a rough implementation of those as a starting point.

Change History (13)

This ticket was mentioned in PR #798 on WordPress/wordpress-develop by iandunn.


4 years ago
#1

  • Keywords has-patch added

This is just a rough & untested sketch of what it could look like, to get feedback on the general concept.

Trac ticket: https://core.trac.wordpress.org/ticket/51966

#2 @azaozz
4 years ago

  • Keywords 2nd-opinion added

I often have trouble getting the right watch or build command going, because they don't seem intuitive or consistent to me.

Same here :)

Frankly I'm thinking the whole "build in one of two places" setup is quite confusing especially for new contributors. Even more when trying to run the tests. Where are they actually running from, build or src, or... sometimes from here, other times from there..? :)

Even more confusing/error prone is when doing svn up followed by grunt build. Then the old, outdated build files are still left in /src. So if you happen to run WP or tests from /src you end up with a mixture of old "built" files (js, css, and some php) mixed with new source files. A mess...

The patch at https://github.com/WordPress/wordpress-develop/pull/798 looks good and makes it easier/better. The inline docs will help too. Thinking it shouldn't stop there though.

Seems the best way to fix the above "mess" would be to always build in both places, build and src. That may be a bit slower, but will simplify a lot of things and prevent a lot of confusion and headaches. Actually it looks like it won't be slower, can build to src and then copy all built files to build. Looking at Gruntfile.js this is pretty easy to do there but will need to fix/tweak Webpack a bit.

Do you think this is a good idea? If we do that, then we can change/simplify/deprecate a lot more of the build workflow and commands, and make it simple again.

Last edited 4 years ago by azaozz (previous) (diff)

#3 @iandunn
4 years ago

I love that! Having just grunt watch and grunt build (and npm run build, npm run watch) would really simplify this.

Leaving 2nd-opinion for a few others to chime in, since it'd be a big change
cc @netweb, @desrosj, @omarreiss, @atimmer, @johnbillion

#4 @atimmer
4 years ago

Recalling our thinking
I spend some time remembering what we were trying to do. The reason for building to src was that we identified that for some WordPress contributors, having to run a watch task to make PHP changes would be a non-starter. I now think that we made a mistake, but that it might have been right within the thinking of that time. Having build & src is too confusing, I even have trouble recalling how it works, which is not a good sign.

So fundamentally, a proper solution would have one folder to run from.

Different approaches
This 'proper solution' results in a philosophical conflict. The WordPress PHP has long had a philosophy of zero-installs. Cloning the repository means it works. The JavaScript world has long had a philosophy of having a build step before it works. I used to be a sole proponent of lean and mean repositories. Only commit the absolute necessities and have a build step producing pristine artifacts that can be run.

My current understanding is that both are a tradeoff with different qualities. The package manager Yarn has recognised this and supports zero-installs very well now: https://yarnpkg.com/features/pnp#fixing-node_modules. "The generated .pnp.js file can be committed to your repository as part of the Zero-Installs effort, removing the need to run yarn install in the first place."

It is not for me to say, but I have a feeling that zero-installs fit better with WordPress's philosophies. Zero-installs are decidedly more accessible as they require less tooling on the part of a new contributor. But it might be that that ship has sailed with Gutenberg.

Band-aid
Having said all that, I think @azaozz's proposal is a proper band-aid as long as the performance is acceptable. So 👍🏻 for simply doing that.

#5 @iandunn
4 years ago

That's really insightful, thanks! I hadn't heard about Zero Installs before, but I think that's a really interesting idea. I'm wondering if something like this would work:

  • Remove the build/ folder entirely.
  • All .php files in src/ stay the same. There's no longer a need to copy them over to build/.
  • Keep src/js the way it is
  • (optionally) create a src/sass folder (cc @helen, @melchoyce)
  • src/js files that don't require any preprocessing -- like dashboard.js -- could be removed from src/js. Then changes to them could be made directly to src/wp-admin/js/bashboard.js without having to worry about a build step.
  • We'd add built files like src/wp-admin/js/foo.js to .gitignore, so that devs don't have to locally manage duplicate git add's etc.
  • We'd setup a A GitHub action to automatically compile & commit the built files to the repo on a post-push hook, like @ryelle did for TwentyTwentyOne.

So the typical dev workflow would be:

  1. If you're making changes to processed JS or SASS files, then you npm run watch. Otherwise, you don't have to run any watch task at all.
  2. Make changes to the files
  3. If they're processed JS/SASS files, watch would compile them from src/js into src/wp-admin/js, src/wp-includes/js etc.
  4. git add the PHP, src/js , or src/sass files. The built files won't be tracked locally, so you won't get bugged by them.
  5. git push
  6. If any src/js or src/sass files were pushed, the GH action would compile and commit them to the PR.

Committers could wget {PR URL}.diff, and commit that. It'd include both src/js and src/wp-admin/js files.

The release process would bundle up all of src/ but ignore src/js and src/sass.

#6 @ryelle
4 years ago

(optionally) create a src/sass folder

The color schemes use sass, but we can leave the current structure (wp-admin/css/colors/*.scss) in place. They're already built in src if you use the dev commands. Currently there's no plan to use Sass anywhere else, if anything we're moving away from it (unless Gutenberg has other plans for core). The PostCSS process will probably be expanded on, but I don't think that means we need a separate CSS source folder.

It looks like the last two default themes have also come into core with build processes, so we might want something like yarn's workspaces to centrally manage building those, too.

This ticket was mentioned in Slack in #core by thewebprincess. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by laura. View the logs.


4 years ago

#9 @isabel_brison
4 years ago

Remove the build/ folder entirely.

Do any dev environment setups rely on this folder currently? Would be good what its original purpose was before removing it.

Also, something to consider given WP keeps moving in a more JS-heavy direction is that JS projects typically are structured with separate src and build folders where only src is version controlled and build generated as needed for production packages or local development (there may be different versions for each, e.g. the development build will likely include sourcemaps that won't be added to the production build). Going in a completely different direction to this will be confusing to JS devs looking to contribute to the project.

We'd setup a A GitHub action to automatically compile & commit the built files to the repo on a post-push hook, like @ryelle did for TwentyTwentyOne.

I'm probably missing something obvious, but why do we need to commit built files to the repo?

I have a feeling that zero-installs fit better with WordPress's philosophies. Zero-installs are decidedly more accessible as they require less tooling on the part of a new contributor.

This is definitely an interesting idea, but I suspect the biggest hurdle for new contributors is getting their local environment set up, and running a build script would be easy in comparison. It would be great to have some data on this though!

#10 @iandunn
4 years ago

[know] what its original purpose was before removing it.

AFAIK, before #43055 `build/` was only necessary for production tasks like minification. Most contribs ran from src locally, since it was simpler/faster.

PHP tests used to run from build, but are now running from src again per #51734.

This wouldn't remove any of the production functionality, it'd just simplify the dev workflow.

...any setups rely on [build/] currently? ...

I'm not aware of any, but if there are, then it should be a straight-forward update. All of the files they'd need would still exist, just in a different location.

JS projects typically are structured ... Going in a completely different direction to this will be confusing to JS devs

Can you explain more about that?

If someone clones the repo and runs the watch command, I think everything would still work as they expected. The only difference is that it'd also work if they didn't run it. We'd definitely want clear docs, though.

The current direction has its own costs, like the unnecessary complexity/fragility and delays that tooling introduces.

Most JS projects don't have a huge PHP component, but WordPress will for the foreseeable future. I feel like the needs of JS devs should be balanced with the needs of PHP devs.

why do we need to commit built files to the repo?

It makes it so that contributors don't need any tooling to get started, and don't need any when making changes to PHP or vanilla JS files.

biggest hurdle for new contributors is getting their local environment set up

I agree that's a problem too, but I don't think it makes this any less of a problem.

Experienced contributors run into problems too, the toolchain introduces a lot of failure points. (Not all of those are directly related to this, but removing unnecessary complexity removes some of those failure points.)

Even when the tools work perfectly, they're still slow.


If there's not a consensus on the zero-install approach, though, we could definitely do the copy-everything approach for now, and leave zero-install for future discussions.

#11 @isabel_brison
4 years ago

...any setups rely on [build/] currently? ...

I'm not aware of any, but if there are, then it should be a straight-forward update.

So I poked around a bit and this is what I found:

wp-env allows for specifying a local version of WP to build the dev environment from (https://github.com/WordPress/gutenberg/blob/trunk/packages/env/README.md#local-wordpress-develop--current-directory-as-a-plugin). The docs example uses build/, and in my testing I wasn't able to get it running from src/ at all. (It fails with Error: 'wp-config.php' not found even though I did have a wp-config.php in the src folder.)

I'm sure this is fixable, but not sure how long it'll take to fix, so in the meantime it would be good to continue to provide a build/ folder. (wp-env is the only automated setup I know of that allows to build an environment from both Core and Gutenberg development directories, so pretty useful to debug cross-codebase issues.)

Relatedly, the script that runs the Gutenberg e2e test suite on Core (https://github.com/WordPress/wordpress-develop/blob/master/tests/gutenberg/run.js) uses wp-env so also relies on build/ (at least until we can fix wp-env to work with src/)

JS projects typically are structured ... Going in a completely different direction to this will be confusing to JS devs

Can you explain more about that?

This is mostly my perception as a dev with more experience in JS/front end than PHP. The running from src/ thing did not seem obvious to me initially and I've since chatted with other JS contributors who were also surprised by this approach. Granted, it's easy enough to understand once the reasoning is explained, so if running from src/ is beneficial to PHP devs, let's do it! But we should be aware that some folks won't be expecting it and provide clear documentation about it.

Experienced contributors run into problems too

As someone who has spent a lot of time debugging build issues, I'm very aware of this :( and agree reducing complexity is a good thing to do wherever possible!

I think the copy-everything approach is the best solution for now, until we can make sure no one is relying on the build/ folder. Also so as to not blow out the initial scope of this ticket too much. Simplifying the build task names is already a great DX improvement so would be good to get that through soon, and then we can work on the zero-install approach as a follow up.

#12 @iandunn
4 years ago

in the meantime it would be good to continue to provide a build/ folder

Ah, that's a good find, thanks!

Since wp-env is an official project, I think it'd make sense to update it as part of any "zero-install" refactor done here. It might even be easier to fix if Core had a simpler setup.

I'd want to see broader support before doing a refactor, though, so unless others are interested, then I agree that the copy approach would be good to do for now.

This ticket was mentioned in Slack in #core by isabelbrison. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.