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

Webpack Helpers v1 Beta: Webpack 5, addFilter() #205

Open
wants to merge 83 commits into
base: main
Choose a base branch
from
Open

Webpack Helpers v1 Beta: Webpack 5, addFilter() #205

wants to merge 83 commits into from

Conversation

kadamwhite
Copy link
Contributor

@kadamwhite kadamwhite commented Apr 29, 2022

This PR changes a few small things.... 😅

  • Update to support Webpack 5 and Webpack DevServer 4
  • Rip out the filterLoaders system in favor of an addFilter more WP-style system I came up with while discussing an issue with @tfrommen
  • Adjust or change a number of loaders and plugins for Webpack 5 compatibility
  • Introduce a sourcemap loader and a few updated configuration options from the latest versions of CRA.

Known issues

  • Production asset manifest is getting auto in the paths due to a publicPath change; need to check that.
  • Need to use this fork of fix-style-only-plugin, since the original is not Webpack 5-compatible.
  • ESLint is now handled via a plugin, the loader is deprecated.
  • Should we review the PostCSS default configuration? This is what CRA does now:
      options: {
        postcssOptions: {
          // Necessary for external CSS imports to work
          // https://github.com/facebook/create-react-app/issues/2677
          ident: 'postcss',
          config: false,
          plugins: [
            'postcss-flexbugs-fixes',
            [
              'postcss-preset-env',
              {
                autoprefixer: {
                  flexbox: 'no-2009',
                },
                stage: 3,
              },
            ],
            // Adds PostCSS Normalize as the reset css with default options,
            // so that it honors browserslist config in package.json
            // which in turn let's users customize the target behavior as per their needs.
            'postcss-normalize',
          ]
        }
    
    It's a little more straightforward than our current system, since it doesn't wrap values inside function calls and may not require as many filter seams. Curious for opinions.

Copy link
Contributor

@tfrommen tfrommen left a comment

Choose a reason for hiding this comment

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

This is some great work! I'd like to test this on one or two projects, but here's some ad hoc feedback.

@kadamwhite
Copy link
Contributor Author

v1.0.0-beta.2 released into the wild

  • Remove preset/prod/stylesheet-loaders and preset/dev/stylesheet-loaders hooks in favor of passing environment as second argument to preset/stylesheet-loaders hook
  • Always use 8-char content hashes in filenames by default
  • Fix "auto" publicPath issue (in beta.1)

@Sephsekla
Copy link
Contributor

This is really awesome! Just gave it a quick "update and see what happens" test locally on a project.

Build

Using the production preset, things were pretty smooth - one deprecation nag around a destructured import, and I needed to specifically install prop-types, which wasn't an explicit dependency before. After a couple of 10 second fixes, I'm seeing substantial speed improvements.

Dev Server

DevServer seems to have more breaking changes, at least for our setup. I'll have to look into it further when I have time, but our array of entries mapped to devServers are no longer happy to use the same port: Error: Unique ports must be specified for each devServer option in your webpack configuration. I've seen this pattern on a couple of projects, so would be good to figure out a rock solid replacement.

Relevant part of the config is here

devServer
	return choosePort( 9090 ).then( ( port ) => entries.map( ( entry ) => {
		return presets.development( {
			...common,
			devServer: {
				...devServer,
				port,
			},
			...entry,
			output: {
				...entry.output,
				publicPath: `https://${ host }:${ port }/${ entry.name.toLowerCase() }/`,
			},
		} );
	} ) );
};

where each item in entries looks something like

entrypoints
const example = {
	name: 'example',
	entry: {
		'lorem': filePath( 'content/plugins/example/assets/src/index.js' ),
	},
	output: {
		path: filePath( 'content/plugins/example/assets/build' ),
	},
};

Something like this seems to work, with a couple of angry messages about running webpack twice for some entrypoints, but I haven't yet had a chance to dig into it too much.

potential devServer
return entries.map( ( entry, index ) =>
		choosePort( 9090 + index ).then( ( port ) => presets.development( {
			...common,
			devServer: {
				...devServer,
				port,
			},
			...entry,
			output: {
				...entry.output,
				publicPath: `https://${ host }:${ port }/${ entry.name.toLowerCase() }/`,
			},
		} ) )
	);

@kadamwhite kadamwhite added this to the 1.0 milestone Jun 17, 2022
@kadamwhite
Copy link
Contributor Author

Summing up a long thread in Slack with @kucrut that we're running into this: webpack/webpack-dev-server#2792 when using multiple entries in one config. dependOn works, in Dzikri's testing runtimeChunk didn't work as advertised in that thread. We may also want to drop the hashes from dev config entries in dev server mode. We may also want to evaluate whether we should set a host value on the devServer object when we are transforming that in the dev preset.

@tfrommen
Copy link
Contributor

We may also want to evaluate whether we should set a host value on the devServer object when we are transforming that in the dev preset.

What does that mean exactly?

Quite a few Altis projects are using the actual "local", that is, altis.dev project URL, like so:

	const host = 'my-project.altis.dev';

	const devServer = {
		allowedHosts: [
			`.${ host }`,
		],
		host,
		https: {
			key: readFileSync( filePath( 'vendor/altis/local-server/docker/ssl.key' ) ),
			cert: readFileSync( filePath( 'vendor/altis/local-server/docker/ssl.cert' ) ),
		},
	};

Are we planning on supporting that still?

@kadamwhite
Copy link
Contributor Author

@tfrommen I don't think we've made any changes which would break that usage. The intent is absolutely to support that.

This resolves an issue where multi-config entries would only output
a manifest containing the entries from the first exported config.

The origin of this issue is that we cannot define a devServer on
every multi-config array, or DevServer v4 will error. But without
defining a devServer we did not usually have the information
needed to deduce a publicPath, and therefore did not insert the
manifest plugin in subsequent configs.

Remove test for not setting manifest plugin
contenthash is not recommended in dev mode, and hash, the value we
previously used, has been renamed fullhash for clarity.
kadamwhite added 10 commits July 1, 2022 09:42
…er changes

You need to manually include the runtimeChunk if you do this.
…e output folder

This permits minimal-config multi-config dev builds which stack multiple config output into one manifest.

Without a patch like this, the second dev config would get the publicPath "auto".
@mattheu
Copy link
Member

mattheu commented Aug 11, 2022

I see there has been some discussion here on some of the issues i've run into... and maybe some fixes too. But I'll note my workarounds in case they're helpful.

  • Multiple webpack configs are not supported. But if you name them e.g. theme, you can run an individual one like --config-name=theme. I set up some aliases in my package.json scripts like npm run start:theme. There is a bit more overhead for the engineer in this, but for our project this was more performant and typically you don't need to run them both at once.
  • Single config but multiple JS files emitted. Need to set in webpack config optimization: { runtimeChunk: 'single' } and then ensure this is loaded once on the page, and set as a dependency of any JS files I'm loading. I have a custom asset loader function that wraps the HM Asset_Loader library to ensure this is done.

@kadamwhite
Copy link
Contributor Author

Multiple webpack configs are not supported. But if you name them e.g. theme, you can run an individual one like --config-name=theme. I set up some aliases in my package.json scripts like npm run start:theme. There is a bit more overhead for the engineer in this, but for our project this was more performant and typically you don't need to run them both at once.

This works well, and I always recommend labeling each config with a name so that you can do things this way. The issue with that approach happens when multiple configurations build to the same directory. We support doing so, with a shared manifest (#154), but imagine this scenario:

  • Configuration editor builds these bundles:
    • editor.js
    • editor.css
  • Configuration frontend builds these bundles:
    • frontend.js
    • frontend.css
    • big-script-for-that-one-complex-page.js

The production manifest would look like this:

{
  "editor.js": "./editor.hash.js",
  "editor.css": "./editor.hash.css",
  "frontend.js": "./frontend.hash.js",
  "frontend.css": "./frontend.hash.css",
  "big-script-for-that-one-complex-page.js": "./big-script-for-that-one-complex-page.hash.js"
}

If you then run npm run dev -- --config-name=editor, you'll end up with a development manifest like this:

{
  "editor.js": "http://localhost:8080/editor.js"
}

The way asset loader works now, if you use

$active_manifest = get_active_manifest( [ 'dev-manifest.json', 'prod-manifest.json' ] );

Asset_Loader\enqueue_asset( $active_manifest, 'editor.js' );

Asset_Loader\enqueue_asset( $active_manifest, 'frontend.js' );

then editor.js will get loaded, but frontend.js will fail (silently!) because it isn't present in the dev manifest.

If we use --config-name heavily, given that we do have a number of projects that build multiple configurations to a single folder, we'd probably need to also adjust the enqueue_asset function to look through each manifest in turn and enqueue the first located version of the requested asset, so that you could fall back to the prod build if the script you need to load isn't available in the dev server.

@mattheu
Copy link
Member

mattheu commented Aug 11, 2022

Something that was asked on a call. If you have to run multiple configs individually, can you just open up a new terminal tab and run the other config over there? And the answer is kind of. It works but hot module reloading fails for the second JS file loaded. I tried loading both only a single runtime JS file, and also multiple but no luck. Maybe there is a workaround?

Pin block-editor-hmr to include a bugfix for a regression in files which do not export settings
@kadamwhite
Copy link
Contributor Author

Remaining tasks for 1.0:

  • Update all dependencies
  • Pare bundled plugins down to a bare minimum
  • Bundle the WordPress Dependency Extraction webpack plugin
  • Set optimization: { runtimeChunk: 'single' } in dev default factory
  • May need to update Asset Loader to account for how to use runtimeChunk and extracted dependencies
  • Stretch goal: bundle and document better helpers for single-block HMR usage, like how we build a single editor file for each block rather than having one editor.js which defines multiple blocks (using modern block.json pattern)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants