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

Calypso Build Package: Add webpack.config.js #31940

Merged
merged 5 commits into from
Apr 3, 2019

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Apr 1, 2019

Changes proposed in this Pull Request

See pafL3P-oe-p2:

Problem: Webpack config re-use is suboptimal

While we’re able to re-use parts of Calypso’s webpack config thanks to the shards concept (i.e. small files each containing specific parts of the config), the resulting webpack.config.js files are still somewhat verbose and redundant. The shards, as we are using them now, have never been the envisioned be-all, end-all, goal for an interface of a re-usable webpack config -- we even had a call with @blowery from +teamcalypsop2 on some options there (tl;dr: It’s complicated).

Solution: Parametrized `@automattic/calypso-build/webpack.config.js`

I think that as an intermediate step, it makes sense to add a parametrized webpack.config.js to @automattic/calypso-build that can then be less redundantly instantiated by consumers. The interface is still going to be suboptimal (essentially the union of all shards’ parameters), but IMO at least not worse than directly using the shards. Thanks to package versioning, we can release a new version featuring a better interface once we come up with one (and being used by a variety of different projects should help inform that interface).

The file used in here is essentially webpack.config.extensions.js minus the entry point magic, and minus CopyWebpackPlugin (about which yarn complained that it wasn't installed 🙄 -- but we might be able to get rid of that anyway, see Automattic/jetpack#11801).

Testing instructions

nvm use
cd packages/calypso-build
npm link
cd ../../
cd ../jetpack # Or wherever your local copy of Jetpack is
git checkout update/use-calypso-build-webpack-config
nvm use 10.15.2
yarn distclean && yarn
npm link @automattic/calypso-build
yarn build-extensions

Verify that blocks are built in your Jetpack directories _inc/blocks/ folder.

Follow-up (this PR or separately)

Use this config file to build o2 blocks.

/cc @sirreal @simison

@ockham ockham added Jetpack [Status] In Progress [Goal] Gutenberg Working towards full integration with Gutenberg labels Apr 1, 2019
@ockham ockham self-assigned this Apr 1, 2019
@matticbot
Copy link
Contributor

'output-path': outputPath = path.join( __dirname, 'dist' ),
'output-filename': outputFilename = '[name].js',
'output-libary-target': outputLibraryTarget = 'window',
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This choice of parametrization is supposed to organically tap into Webpack's way of accepting command line args. We should be able to use this config file with Webpack's CLI, and override these args, e.g. webpack --config ./webpack.config.js --output-filename="[chunk].js".

(Meaning I'm trying to essentially re-use Webpack's API without adding any new constraints.)

@ockham ockham requested a review from a team as a code owner April 2, 2019 11:53
@ockham ockham force-pushed the add/webpack-config-to-calypso-build-package branch from 154d718 to 9955795 Compare April 2, 2019 12:31
@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 2, 2019
@ockham ockham requested a review from a team April 2, 2019 12:32
@ockham
Copy link
Contributor Author

ockham commented Apr 2, 2019

FYI @jeffersonrabb This might provide a nicer interface than using individual Webpack config shards for Newspack. See Automattic/jetpack#11802 for how to use it 🙂

optimization: {
minimize: ! isDevelopment,
minimizer: Minify( {
cache: process.env.CIRCLECI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we expect to parametrize all these process.env variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been discussing that, e.g. using the function's env arg. We decided to merge this for an i1 since it seems to work well enough (see Automattic/jetpack#11802), and tackle those chores later.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get an alpha published and see how this plays out 🚀

@ockham ockham force-pushed the add/webpack-config-to-calypso-build-package branch from 9955795 to 9cab7af Compare April 3, 2019 13:25
@ockham ockham merged commit 8f4bc0b into master Apr 3, 2019
@ockham ockham deleted the add/webpack-config-to-calypso-build-package branch April 3, 2019 13:42
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg Jetpack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants