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: Allow changing file entry and output of build scripts #14891

Closed
helgatheviking opened this issue Apr 10, 2019 · 16 comments · Fixed by #15982
Closed

Scripts: Allow changing file entry and output of build scripts #14891

helgatheviking opened this issue Apr 10, 2019 · 16 comments · Fixed by #15982
Assignees
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Scripts /packages/scripts [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement.

Comments

@helgatheviking
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I am using the WordPress build scripts to fasttrack my ability to work with Gutenberg. I'm very new to the ideal of webpack, but in my existing plugins using the default src and build file location and naming conventions is not ideal.

The readme does explain that you can extend the webpack config. But if you're very new, that example doesn't help with the file location.

Describe the solution you'd like
Include an example of changing the file name/locations for input and output. I am obviously biased here, but it strikes me as the most likely need for anyone just starting.

Describe alternatives you've considered
I did eventually figure this out, so for anyone else who needs it, saving the following to your project folders root as webpack.config.js seems to do the trick. This changes the input file to /js/src/index.js and the output to /js/dist/index.js

const path = require('path');
const defaultConfig = require("./node_modules/@wordpress/scripts/config/webpack.config");

module.exports = {
  ...defaultConfig,
	entry: {
		index: path.resolve( __dirname, 'js/src', 'index.js' ),
	},
	output: {
		filename: '[name].js',
		path: path.resolve( __dirname, 'js/dist' ),
	},
};

@gziolo gziolo added [Package] Scripts /packages/scripts [Type] Enhancement A suggestion for improvement. [Type] Build Tooling Issues or PRs related to build tooling labels Apr 10, 2019
@gziolo gziolo changed the title Include example for changing file output of build scripts in Readme Scripts: Allow changing file entry and output of build scripts Apr 10, 2019
@gziolo gziolo added Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts labels Apr 10, 2019
@gziolo
Copy link
Member

gziolo commented Apr 10, 2019

@helgatheviking - how about we make it an option for the wp-scripts start and wp-scripts build commands instead? This is something that was raised by @swissspidy and @youknowriad in the past. We agreed that it would be a very good enhancement and common use case.

/cc @nosolosw

@helgatheviking
Copy link
Contributor Author

@gziolo I'm all for whatever way makes it easy for future me. So an option (documented of course!) is perfect.

@alexwong
Copy link

I can take a look at this!

@ockham
Copy link
Contributor

ockham commented May 17, 2019

FWIW, what we're doing in calypso-build is essentially just adding a very thin wrapper around the Webpack CLI tool. This has the advantage that users of calypso-build can pass any arguments that Webpack's CLI supports to it, and they will be automatically handed over to Webpack, without the need to maintain any logic for those arguments in our wrapper.

I'd like to modestly suggest changing wp-scripts build and wp-scripts start accordingly -- I think a lot of the layers and extra utils that are currently involved (getWebpackArgs etc) could be nixed, thus removing some maintenance burden.

(I can give this a stab.)

@youknowriad
Copy link
Contributor

@ockham

As it stands today, wp-scripts build is also a thin wrapper around Webpack CLI, it also supports passing a differnt webpack config. We discussed in #core-js chats that ultimately, we may remove that possibility entirely. Our goal is not to offer a webpack config, you should just use webpack directly if you know how to configure it, the primary goal is to offer an opionated tool for WP plugin/theme authors to build and setup their plugins easily (the less config, the better). We could also imagine changing the underlying bundler used.

Related docs https://developer.wordpress.org/block-editor/packages/packages-scripts/#advanced-usage

That said, we offer a webpack config today and if there are ways to improve the implementation, we're open to it.

@gziolo
Copy link
Member

gziolo commented May 17, 2019

I'd like to modestly suggest changing wp-scripts build and wp-scripts start accordingly -- I think a lot of the layers and extra utils that are currently involved (getWebpackArgs etc) could be nixed, thus removing some maintenance burden.

I think getWebpackArgs exists only to pass additional webpack params more easily:

https://github.com/WordPress/gutenberg/blob/master/packages/scripts/utils/config.js#L25-L32

https://github.com/WordPress/gutenberg/blob/master/packages/scripts/scripts/start.js#L14

@nosolosw should give you more details about it. Otherwise, you might be able to pass all params webpack supports as well and they should just work. The reason we don't cover it in docs was explained by @youknowriad in the previous comments.

@ockham
Copy link
Contributor

ockham commented May 17, 2019

@ockham

As it stands today, wp-scripts build is also a thin wrapper around Webpack CLI, it also supports passing a differnt webpack config. We discussed in #core-js chats that ultimately, we may remove that possibility entirely. Our goal is not to offer a webpack config, you should just use webpack directly if you know how to configure it, the primary goal is to offer an opionated tool for WP plugin/theme authors to build and setup their plugins easily (the less config, the better). We could also imagine changing the underlying bundler used.

Related docs https://developer.wordpress.org/block-editor/packages/packages-scripts/#advanced-usage

That said, we offer a webpack config today and if there are ways to improve the implementation, we're open to it.

I think the implementation could be simplified by essentially replacing packages/scripts/scripts/build.js with something like https://github.com/Automattic/wp-calypso/blob/b18ff5c3b349e8d50dc83f4fd36bcf281c97692d/packages/calypso-build/bin/calypso-build.js

As a side effect, it will allow passing Webpack args like output path and specifying entry points to the command, which I thought was the consensus of this issue, e.g.

wp-scripts build --output-path='./build' editor=./src/editor.js view=./src/view.js

While adding some flexibility here, the tool would still be usable as before, i.e. I think it wouldn't become less of an easy-to-use tool for plugin and theme authors. It would also be a step towards convergence with calypso-build.

Anyway, let me know what you think about this suggestion.

@youknowriad
Copy link
Contributor

What I really care about is the API. I think the ideal API is something like:

wp-scripts build entry1.js entry2.js --output dist

I feel like if we allow any webpack config, it's harping the CLI API a little bit.

@ockham
Copy link
Contributor

ockham commented May 17, 2019

What I really care about is the API. I think the ideal API is something like:

wp-scripts build entry1.js entry2.js --output dist

I think that this format (unnamed entry points) is supported by Webpack OOTB, meaning we'd get it for free as well if we just pass arguments right through to Webpack.

I feel like if we allow any webpack config, it's har[m]ing the CLI API a little bit.

Hmm, why do you think so? I guess the vast majority of users would continue to use the command without any args; some would maybe specify entry points and output path; and only very few would tweak more advanced settings.

(Some of this might be philosophical -- personally, I've concluded that if what I'm doing is essentially running Webpack, I might as well expose its API. Saves me from needing to add extra CLI args as they're requested, and from maintaining layers of extra tooling that I wrap around it.)

@youknowriad
Copy link
Contributor

I agree that it's mainly a philosophical problem:

  • We want to be an opinionated tool without too many options to have a consistent user base, we don't want to be webpack.
  • Can we "limit" what APIs we offer in our CLI. Say only whitelist entry and output?

@ockham
Copy link
Contributor

ockham commented May 17, 2019

Yeah, was about to suggest whitelisting. Still might require a bit of parsing to be able to e.g. recognize different entry point formats (and thus a fatter wrapper than I'd ideally like, but oh well 😅)

@gziolo
Copy link
Member

gziolo commented May 20, 2019

@ockham would you mind sharing a quick proof of concept? It should be easier to discuss based on code than trying to imagine the subtle differences you are proposing. It feels like it’s not that far from what @youknowriad explains on the higher level. The goal is to have reasonable defaults for all projects which operate in WordPress ecosystem with all possible optimizations when developing blocks.

@ockham
Copy link
Contributor

ockham commented May 20, 2019

@gziolo Happy to share a PoC, yep!

BTW, turns out that wp-scripts build actually accepts Webpack args and forwards them to Webpack already -- we're not currently whitelisting any args! This can be e.g. demonstrated by running node_modules/.bin/wp-scripts build --output-filename='./dist/[basename]/index.js'.

@ockham
Copy link
Contributor

ockham commented May 20, 2019

@gziolo Happy to share a PoC, yep!

BTW, turns out that wp-scripts build actually accepts Webpack args and forwards them to Webpack already -- we're not currently whitelisting any args! This can be e.g. demonstrated by running node_modules/.bin/wp-scripts build --output-filename='./dist/[basename]/index.js'.

As a consequence, my PoC is essentially a noop, except that I haven't found out yet how to get a combination of output-filename and output-path to work 😬

FWIW, packages/scripts/scripts/build.js could be simplified as follows:

diff --git a/packages/scripts/scripts/build.js b/packages/scripts/scripts/build.js
index 2c3b9318a..d3e1925e8 100644
--- a/packages/scripts/scripts/build.js
+++ b/packages/scripts/scripts/build.js
@@ -1,18 +1,7 @@
-/**
- * External dependencies
- */
-const { sync: spawn } = require( 'cross-spawn' );
-const { sync: resolveBin } = require( 'resolve-bin' );
-
 /**
  * Internal dependencies
  */
 const { getWebpackArgs } = require( '../utils' );
 
-process.env.NODE_ENV = process.env.NODE_ENV || 'production';
-const { status } = spawn(
-       resolveBin( 'webpack' ),
-       getWebpackArgs(),
-       { stdio: 'inherit' }
-);
-process.exit( status );
+process.argv = [ 'dummy', 'arg', ...getWebpackArgs() ]; // Need to 'pad' two leading args for Webpack to remove
+require( 'webpack-cli' );

@gziolo
Copy link
Member

gziolo commented May 20, 2019

@wordpress/scripts uses this webpack binary:
https://unpkg.com/webpack@4.8.3/bin/webpack.js
which also uses webpack-cli behind the scenes so it doesn't change that much. It should work the same with the remark that you propose to remove the default value production for NODE_ENV in build script.

@gziolo gziolo self-assigned this Jun 4, 2019
@gziolo gziolo added [Status] In Progress Tracking issues with work in progress and removed Needs Dev Ready for, and needs developer efforts labels Jun 4, 2019
@gziolo
Copy link
Member

gziolo commented Jun 5, 2019

I proposed support for multiple entry points in #15982.

I also discovered that it works out of the box as of today with the following command:

$ npx wp-scripts build entry-one=./entry-one.js entry-two=./entry-two.js --output-path=dist

There is no need to override the default webpack config.

iandunn added a commit to WordPress/wordcamp.org that referenced this issue Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Scripts /packages/scripts [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants