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 | 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
andnpm run build:dev
, but the corresponding commands arenpm run watch
andnpm run dev
, instead ofnpm run watch:dev
. npm run build
wrapsgrunt build
, butnpm run dev
wrapsgrunt watch --dev
; there is no correspondinggrunt dev
task.- running
npm run watch --dev
will rungrunt watch
(intobuild/
) instead ofgrunt watch --dev
(intosrc/
). That's becausenpm
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/
andbuild/
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
#2
@
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.
#3
@
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
@
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
@
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 insrc/
stay the same. There's no longer a need to copy them over tobuild/
. - 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 -- likedashboard.js
-- could be removed fromsrc/js
. Then changes to them could be made directly tosrc/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 duplicategit 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:
- 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. - Make changes to the files
- If they're processed JS/SASS files,
watch
would compile them fromsrc/js
intosrc/wp-admin/js
,src/wp-includes/js
etc. git add
the PHP,src/js
, orsrc/sass
files. The built files won't be tracked locally, so you won't get bugged by them.git push
- If any
src/js
orsrc/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
@
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
@
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
@
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
@
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
@
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 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