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

Build Tooling: Configure Webpack to watch only build files #21489

Merged
merged 2 commits into from
Apr 17, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 8, 2020

This pull request seeks to revise the Webpack configuration to instruct the watcher to ignore all files which are outside a package's build output directories. The project is built in such a way that Webpack is responsible only for the part of the build where package output is transformed to be run in a browser. The package build (Babel, etc) is handled separately, where the output is directed to a package's build, build-module, and build-style folder, and then "handed off" to Webpack. Thus, it should be safe to assume that Webpack is only concerned with changes within these files.

The intention with these changes is to resolve cases where an operating system may have limited resources to watch files, either hitting a limit or maxing processor usage.

Related:

Implementation Notes:

Credit to @draganescu for identifying watchOptions.ignored as the configuration for addressing the underlying issue.

Webpack uses anymatch under the hood, which in turn uses micromatch. Refer to documentation for more detail about the syntax of the glob patterns:

Testing Instructions:

Verify there are no regressions in watch behavior. Changing a source file within a package should trigger a Webpack rebuild.

@aduth aduth added [Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts labels Apr 8, 2020
@aduth aduth requested review from draganescu and gziolo April 8, 2020 19:43
@github-actions
Copy link

github-actions bot commented Apr 8, 2020

Size Change: +13.1 kB (1%)

Total Size: 903 kB

Filename Size Change
build/annotations/index.js 3.62 kB +213 B (5%) 🔍
build/api-fetch/index.js 4.01 kB +211 B (5%) 🔍
build/autop/index.js 2.82 kB +238 B (8%) 🔍
build/block-directory/index.js 6.24 kB +203 B (3%)
build/block-editor/index.js 104 kB +1.8 kB (1%)
build/block-editor/style-rtl.css 10.2 kB +4 B (0%)
build/block-editor/style.css 10.2 kB +2 B (0%)
build/block-library/editor-rtl.css 7.22 kB -11 B (0%)
build/block-library/editor.css 7.22 kB -11 B (0%)
build/block-library/index.js 112 kB +1.61 kB (1%)
build/block-library/style-rtl.css 7.15 kB -271 B (3%)
build/block-library/style.css 7.16 kB -272 B (3%)
build/block-library/theme-rtl.css 683 B +14 B (2%)
build/block-library/theme.css 685 B +14 B (2%)
build/block-serialization-default-parser/index.js 1.88 kB +234 B (12%) ⚠️
build/blocks/index.js 57.7 kB +221 B (0%)
build/components/index.js 198 kB +1.87 kB (0%)
build/components/style-rtl.css 16.6 kB +2 B (0%)
build/components/style.css 16.5 kB +2 B (0%)
build/compose/index.js 6.66 kB +447 B (6%) 🔍
build/core-data/index.js 11.1 kB +393 B (3%)
build/data-controls/index.js 1.25 kB +210 B (16%) ⚠️
build/data/index.js 8.43 kB +187 B (2%)
build/date/index.js 5.47 kB +103 B (1%)
build/dom/index.js 3.1 kB +1 B
build/edit-navigation/index.js 3.1 kB +356 B (11%) ⚠️
build/edit-post/index.js 93.5 kB +639 B (0%)
build/edit-site/index.js 10.4 kB +326 B (3%)
build/edit-widgets/index.js 7.53 kB +348 B (4%)
build/editor/index.js 43.6 kB +792 B (1%)
build/element/index.js 4.64 kB +202 B (4%)
build/format-library/index.js 7.29 kB +339 B (4%)
build/hooks/index.js 2.13 kB +208 B (9%) 🔍
build/i18n/index.js 3.56 kB -5 B (0%)
build/is-shallow-equal/index.js 711 B +1 B
build/keyboard-shortcuts/index.js 2.51 kB +213 B (8%) 🔍
build/keycodes/index.js 1.91 kB +211 B (11%) ⚠️
build/list-reusable-blocks/index.js 3.12 kB +134 B (4%)
build/media-utils/index.js 5.28 kB +441 B (8%) 🔍
build/notices/index.js 1.79 kB +220 B (12%) ⚠️
build/nux/index.js 3.4 kB +398 B (11%) ⚠️
build/plugins/index.js 2.67 kB +137 B (5%) 🔍
build/primitives/index.js 1.49 kB -6 B (0%)
build/redux-routine/index.js 2.84 kB +1 B
build/rich-text/index.js 14.8 kB +317 B (2%)
build/server-side-render/index.js 2.67 kB +130 B (4%)
build/shortcode/index.js 1.7 kB -1 B
build/token-list/index.js 1.28 kB +1 B
build/url/index.js 4.01 kB +1 B
build/viewport/index.js 1.84 kB +235 B (12%) ⚠️
build/warning/index.js 1.14 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/priority-queue/index.js 789 B 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

webpack.config.js Outdated Show resolved Hide resolved
@aduth
Copy link
Member Author

aduth commented Apr 9, 2020

Considering an alternative: we could just have the pattern match any file within packages.

  • Advantage: Simpler, less chance we miss something.
  • Disadvantage: It's not necessary to watch changes in src/ files, since those are handled by the first-pass build step.

I'd need to test if micromatch can support negation as part of the nested path. The documented examples seem to indicate it might be possible:

!packages/*/!(src)/**/*

@gziolo
Copy link
Member

gziolo commented Apr 9, 2020

Yes, the proposed alternative is quite nice. If it works it will cover the majority of cases 👍

@aduth
Copy link
Member Author

aduth commented Apr 10, 2020

It seems to work pretty well:

⇒ node            
Welcome to Node.js v12.13.0.
Type ".help" for more information.
> var micromatch = require( 'micromatch' );
undefined
> micromatch.isMatch( 'node_modules/ansi-red/index.js', '!packages/*/!(src)/**/*' );
true
> micromatch.isMatch( 'packages/a11y/src/index.js', '!packages/*/!(src)/**/*' );
true
> micromatch.isMatch( 'packages/a11y/build/index.js', '!packages/*/!(src)/**/*' );
false
> micromatch.isMatch( 'packages/is-shallow-equal/lib/index.js', '!packages/*/!(src)/**/*' );
false

Note: true above means a change in that file would be ignored.

One thing I want to follow-up on is: I seem to recall we have some packages that have files in the root of the package. The pattern above only matches files in a subfolder. is-shallow-equal used to be one of these packages that had files in the root. I want to see if there are others. This could be in combination with an effort to avoid having src/ for packages not intended to be transpiled (wasted time in build). I can probably whip together an ad hoc script to try to find these violations.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Nice, my hope is that all packages that aren't transpiled don't use src subfolder at all. As discussed already, we started using lib only recently for better code organization and easier publishing process with files section in package.json file.

@aduth
Copy link
Member Author

aduth commented Apr 10, 2020

One thing I want to follow-up on is: I seem to recall we have some packages that have files in the root of the package. The pattern above only matches files in a subfolder. is-shallow-equal used to be one of these packages that had files in the root. I want to see if there are others. This could be in combination with an effort to avoid having src/ for packages not intended to be transpiled (wasted time in build). I can probably whip together an ad hoc script to try to find these violations.

Nothing relevant for this pull request. docgen has a src directory but isn't transpiled. Mentioned a while back at #13329 (comment). I probably don't want to mess with it 'til #21238 lands, just to avoid conflicts. I'm not really sure what to make of block-serialization-spec-parser. AFAIK, it's not even used at runtime?

const { join } = require( 'path' );
const { readdirSync, statSync, existsSync } = require( 'fs' );

const PACKAGES_DIR = join( __dirname, '../packages' );

const packages = readdirSync( PACKAGES_DIR ).filter( ( candidate ) =>
	statSync( join( PACKAGES_DIR, candidate ) ).isDirectory()
);

console.log(
	'unnecessary src',
	packages.filter(
		( dir ) =>
			! require( join( PACKAGES_DIR, dir, 'package.json' ) ).module &&
			existsSync( join( PACKAGES_DIR, dir, 'src' ) )
	)
);

console.log(
	'absent src',
	packages.filter(
		( dir ) =>
			require( join( __dirname, '../package.json' ) ).dependencies[
				'@wordpress/' + dir
			] &&
			! require( join( PACKAGES_DIR, dir, 'package.json' ) ).module &&
			! (
				require( join( PACKAGES_DIR, dir, 'package.json' ) ).main || ''
			).startsWith( 'lib/' )
	)
);
 ⇒ node bin/find-violations.js
unnecessary src [ 'docgen' ]
absent src [ 'block-serialization-spec-parser' ]

@gziolo
Copy link
Member

gziolo commented Apr 10, 2020

Nothing relevant for this pull request. docgen has a src directory but isn't transpiled. Mentioned a while back at #13329 (comment). I probably don't want to mess with it 'til #21238 lands, just to avoid conflicts.

Interesting, I was wrong. Fortunately, it doesn't have any impact because it doesn't contain any production code.

@gziolo
Copy link
Member

gziolo commented Apr 16, 2020

Should it be merged?

@aduth
Copy link
Member Author

aduth commented Apr 16, 2020

Should it be merged?

I think it can be. I'm planning to do a wave of merges tomorrow.

@aduth aduth merged commit 4b9a049 into master Apr 17, 2020
@aduth aduth deleted the update/webpack-watch-build branch April 17, 2020 20:02
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants