-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 support for start and build scripts to the package #12837
Conversation
3a3c365
to
34649e5
Compare
6832676
to
9d8e17b
Compare
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.
👍 to the changes. Still have a minor question though.
9d8e17b
to
c6f06ea
Compare
c6f06ea
to
dbeed60
Compare
@youknowriad are we fine to land it as part of 5.0 (Gutenberg)? |
I'm personally fine, but would love some inputs from people that tried building custom plugins and if this solves their need cc @aduth ? |
Discussed in today's #core-js meeting (link requires registration): https://wordpress.slack.com/archives/C5UNMSU4R/p1548165732173600 |
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.
Looks good 👍 Becomes most useful with the default configuration.
|
||
if ( hasWebpackConfig ) { | ||
// Sets environment to production. | ||
process.env.NODE_ENV = 'production'; |
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's a bit strange to me for the script to be explicitly assigning an environment variable, though I think it's likely the most effective way to capture how most configurations would consider the current environment.
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 replicates what Gutenberg does as of today. There is a similar trick used for unit tests where NODE_ENV
is forced to test
.
* Scripts: Add build and start scripts to wp-script * Introduce start and build scripts in scripts package * Use Webpack for builds in scripts package * Add documentation for start and build commands * Address feedback from review
* Scripts: Add build and start scripts to wp-script * Introduce start and build scripts in scripts package * Use Webpack for builds in scripts package * Add documentation for start and build commands * Address feedback from review
Description
Inspired by work done by @griffbrad in #12685 where he tries to extract the big chunk of Webpack config for the usage outside of Gutenberg. This PR offers two new
@wordpress/scripts
commands to help with maintaining ESNext codebase:wp-scripts build
wp-scripts start
This PR doesn't provide the default Webpack config. I'm leaving it for #12685.
It uses Parcel by default to make it easy to start using build tools for plugin developers. It should work for plugins without any Babel config provided. However, it would be nice to recommend using@wordpress/babel-preset-default
to get everything aligned with core development.Edit: I removed Parcel integration from this PR as it can't be zero-config for WordPress development because of the need to provide Babel config override manually. Parcel also doesn't allow to list the packages which won't be bundled - we need that for all WordPress packages that are being already served by WordPress core.
For advanced usage it uses Webpack when accompanying config file is provided using on of the following:
--config
CLI optionwebpack.config.js
webpack.config.babel.js
This PR is still in progress. It needs the following:Tasks finished:@wordpress/scripts
package@wordpress/scripts
to be updatedHow has this been tested?
npm run dev
./node_modules/.bin/wp-scripts start
npm run build
./node_modules/.bin/wp-scripts build