Page MenuHomePhabricator

[Code review] Make the Popups Webpack configuration consistent with MobileFrontend
Closed, ResolvedPublic

Description

Review Hygiene: copy MobileFrontend Webpack learnings.

Acceptance criteria

  • The change doesn't break anything
  • The configuration is consistent with MobileFrontend
  • The patch is merged

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptDec 21 2018, 7:05 PM
Restricted Application added subscribers: Gilles, Aklapper. · View Herald Transcript

Change 464849 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Hygiene: copy MobileFrontend Webpack learnings

https://gerrit.wikimedia.org/r/464849

Change 464849 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: copy MobileFrontend Webpack learnings

https://gerrit.wikimedia.org/r/464849

Jdlrobson added a subscriber: Jdlrobson.

@Niedzielski Aside from the expected differences, and a minor difference between isProduction/prod variable name (I prefer the former) I'm wondering if webpack.NamedModulesPLugin can be dropped and optimisations. namedModules used/not used?

diff webpack.config.js ~/git/core/extensions/Popups/webpack.config.js  --suppress-common-lines -u
--- webpack.config.js	2019-01-03 08:28:38.000000000 -0800
+++ /Users/jrobson/git/core/extensions/Popups/webpack.config.js	2019-01-03 11:53:17.000000000 -0800
@@ -1,16 +1,24 @@
-/* eslint-env node, es6 */
-const
+/* global __dirname, process */
+const path = require( 'path' ),
+	webpack = require( 'webpack' ),
 	CleanPlugin = require( 'clean-webpack-plugin' ),
-	glob = require( 'glob' ),
-	path = require( 'path' ),
-	prod = process.env.NODE_ENV === 'production',
-	// The output directory for all build artifacts. Only absolute paths are accepted by
-	// output.path.
-	distDir = path.resolve( __dirname, 'resources/dist' ),
-	// The extension used for source map files.
-	srcMapExt = '.map.json';
+	PUBLIC_PATH = '/w/extensions/Popups',
+	isProduction = process.env.NODE_ENV === 'production';
 
-module.exports = {
+const reduxPath = isProduction ?
+	'node_modules/redux/dist/redux.min.js' :
+	'node_modules/redux/dist/redux.js';
+
+const reduxThunkPath = isProduction ?
+	'node_modules/redux-thunk/dist/redux-thunk.min.js' :
+	'node_modules/redux-thunk/dist/redux-thunk.js';
+
+const distDir = path.resolve( __dirname, 'resources/dist' );
+
+// The extension used for source map files.
+const srcMapExt = '.map.json';
+
+const conf = {
 	// Apply the rule of silence: https://wikipedia.org/wiki/Unix_philosophy.
 	stats: {
 		all: false,
@@ -20,75 +28,31 @@
 
 	// Fail on the first build error instead of tolerating it for prod builds. This seems to
 	// correspond to optimization.noEmitOnErrors.
-	bail: prod,
+	bail: isProduction,
 
 	// Specify that all paths are relative the Webpack configuration directory not the current
 	// working directory.
 	context: __dirname,
 
-	// A map of ResourceLoader module / entry chunk names to JavaScript files to pack. E.g.,
-	// "mobile.startup" maps to src/mobile.startup/mobile.startup.js. The JavaScript entry could be
-	// named simply "index.js" but the redundancy of "[name].js" improves presentation and search-
-	// ability in some tools. Entry names are tightly coupled to output.filename and extension.json.
-	entry: {
-		// Note that the tests.mobilefrontend module only exists inside Special:JavaScriptTest test
-		// runs. It provides scaffolding (template mocks) and does not appear inside the
-		// ResourceLoader startup module, so does not use the `mobile.` prefix that other modules
-		// do. This is consistent with other test related artifacts. E.g.,
-		// test.mediawiki.qunit.testrunner and test.sinonjs.
-		// The glob module is used to ensure that all tests inside the folder (minus stubs) are
-		// caught and run to ensure we don't forget to register new tests.
-		'tests.mobilefrontend': glob.sync( './tests/node-qunit/*/*.test.js' ),
-
-		// mobile.startup.runtime: reserved entry for the Webpack bootloader
-		// optimization.runtimeChunk. Without a distinct runtime chunk, it's instead bundled into
-		// each entry which is inefficient. This chunk should only change when Webpack or this
-		// configuration changes.
-
-		'mediawiki.template.hogan': './src/mediawiki.template.hogan/mediawiki.template.hogan.js',
-		'mobile.startup': './src/mobile.startup/mobile.startup.js'
-	},
+	entry: { index: './src' },
 
-	// tests.mobilefrontend has additional dependencies but they're provided externally. This code
-	// can be removed if tests.mobilefrontend is removed.
-	externals: [ 'jquery', 'jsdom', 'oojs', 'sinon', 'qunit', 'fs', 'path' ],
-	resolve: {
-		alias: {
-			// This avoids leaking unnecessary code into the webpack test build
-			'./mockMediaWiki': path.resolve( __dirname, 'tests/node-qunit/utils/blank.json' )
-		}
-	},
 	optimization: {
-		// Generate a single Webpack bootstrap chunk for ResourceLoader modules to share.
-		// This will be packaged inside the mobile.startup module which should be a dependency for
-		// all modules.
-		// The inefficient  alternative is for each module to bundle its own runtime.
-		// The terms bootloader and runtime are used interchangeably.
-		runtimeChunk: { name: 'mobile.startup.runtime' },
-
 		// Don't produce production output when a build error occurs.
-		noEmitOnErrors: prod
+		noEmitOnErrors: isProduction
 	},
 
 	output: {
-		// Specify the destination of all build products.
+		// The absolute path to the output directory.
 		path: distDir,
+		devtoolModuleFilenameTemplate: `${PUBLIC_PATH}/[resource-path]`,
 
-		// Store outputs per module in files named after the modules. For the JavaScript entry
-		// itself, append .js to each ResourceLoader module entry name. This value is tightly
-		// coupled to sourceMapFilename.
+		// Write each chunk (entries, here) to a file named after the entry, e.g.
+		// the "index" entry gets written to index.js.
 		filename: '[name].js',
 
 		// Rename source map extensions. Per T173491 files with a .map extension cannot be served
 		// from prod.
-		sourceMapFilename: `[file]${srcMapExt}`,
-
-		// Expose the module.exports of each module entry chunk through the global
-		// mfModules[name].
-		// This is useful for debugging. E.g., mfModules['mobile.startup'] is set by the
-		// module.exports of mobile.startup.js.
-		library: [ 'mfModules', '[name]' ],
-		libraryTarget: 'this'
+		sourceMapFilename: `[file]${srcMapExt}`
 	},
 
 	// Accurate source maps at the expense of build time. The source map is intentionally exposed
@@ -97,24 +61,75 @@
 	devtool: 'source-map',
 
 	plugins: [
-		// Delete the output directory on each build.
-		new CleanPlugin( distDir, { verbose: false } )
+		new CleanPlugin( distDir, { verbose: false } ),
+
+		// To generate identifiers that are preserved over builds, webpack supplies
+		// the NamedModulesPlugin (generates comments with file names on bundle)
+		// https://webpack.js.org/guides/caching/#deterministic-hashes
+		new webpack.NamedModulesPlugin()
 	],
 
 	performance: {
 		// Size violations for prod builds fail; development builds are unchecked.
-		hints: prod ? 'error' : false,
+		hints: isProduction ? 'error' : false,
 
 		// Minified uncompressed size limits for chunks / assets and entrypoints. Keep these numbers
-		// up-to-date and rounded to the nearest 10th of a kilobyte so that code sizing costs are
+		// up-to-date and rounded to the nearest 10th of a kibibyte so that code sizing costs are
 		// well understood. Related to bundlesize minified, gzipped compressed file size tests.
 		// Note: entrypoint size implicitly includes the mobile.startup.runtime entry.
-		maxAssetSize: 52.3 * 1024,
-		maxEntrypointSize: 53.7 * 1024,
+		maxAssetSize: 40 * 1024,
+		maxEntrypointSize: 40 * 1024,
+
+		// The default filter excludes map files but we rename ours.
+		assetFilter: ( filename ) => !filename.endsWith( srcMapExt )
+	},
 
-		// The default filter excludes map files but we rename ours. Also, any modules prefixed with
-		// "tests." are excluded from performance checks as they are not shipped to end users.
-		assetFilter: ( filename ) => !filename.startsWith( 'tests.' ) &&
-			!filename.endsWith( srcMapExt )
+	resolve: {
+		alias: {
+			redux: path.resolve( __dirname, reduxPath ),
+			'redux-thunk': path.resolve( __dirname, reduxThunkPath )
+		}
+	},
+
+	module: {
+		rules: [
+			{
+				test: /\.js$/,
+				exclude: /node_modules/,
+				use: {
+					loader: 'babel-loader',
+					options: {
+						cacheDirectory: true
+					}
+				}
+			},
+			{
+				test: /\.svg$/,
+				loader: 'svg-inline-loader',
+				options: {
+					removeSVGTagAttrs: false // Keep width and height attributes.
+				}
+			}
+		]
 	}
 };
+
+// Production settings.
+// Define the global process.env.NODE_ENV so that libraries like redux and
+// redux-thunk get development code trimmed.
+// Enable minimize flags for webpack loaders and disable debug.
+if ( isProduction ) {
+	conf.plugins = conf.plugins.concat( [
+		new webpack.LoaderOptionsPlugin( {
+			minimize: true,
+			debug: false
+		} ),
+		new webpack.DefinePlugin( {
+			'process.env': {
+				NODE_ENV: JSON.stringify( 'production' )
+			}
+		} )
+	] );
+}
+
+module.exports = conf;
Jdlrobson updated the task description. (Show Details)Jan 3 2019, 7:58 PM

Change 464853 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Hygiene: remove unused Webpack plugins

https://gerrit.wikimedia.org/r/464853

isProduction/prod variable name (I prefer the former)

Works for me.

I'm wondering if webpack.NamedModulesPLugin can be dropped and optimisations. namedModules used/not used?

I'll get back to you.

Change 464853 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: remove unused Webpack plugins

https://gerrit.wikimedia.org/r/464853

Jdlrobson closed this task as Resolved.Jan 3 2019, 10:27 PM
Niedzielski reopened this task as Open.Jan 4 2019, 4:01 PM

Reopening to track feedback requested.

Change 482318 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename Webpack variable

https://gerrit.wikimedia.org/r/482318

Change 482318 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename Webpack variable

https://gerrit.wikimedia.org/r/482318

Change 482328 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Hygiene: replace deprecated Webpack plugin

https://gerrit.wikimedia.org/r/482328

I'm wondering if webpack.NamedModulesPLugin can be dropped and optimisations. namedModules used/not used?

Great note! NamedModulesPlugin was replaced. I see identical outputs when optimisations.namedModules = true is used instead.

It seems like an improvement to me. If you like this change, I will make it in MobileFrontend. If you dislike it, I will remove NamedModulesPlugin and optimisations.namedModules.

Change 482329 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Update: make Webpack build products stable

https://gerrit.wikimedia.org/r/482329

Change 482329 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Update: make Webpack build products stable

https://gerrit.wikimedia.org/r/482329

Niedzielski closed this task as Resolved.Jan 4 2019, 6:41 PM

Change 482328 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: replace deprecated Webpack plugin

https://gerrit.wikimedia.org/r/482328