WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 4 weeks ago

#51966 new enhancement

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

Reported by: 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 (8)

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


6 weeks ago

  • 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
5 weeks 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 5 weeks ago by azaozz (previous) (diff)

#3 @iandunn
5 weeks 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
5 weeks 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
5 weeks 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
5 weeks 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.


5 weeks ago

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


4 weeks ago

Note: See TracTickets for help on using tickets.