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.

Sign up for GitHub

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

Jump to bottom

fix(web): fix styles vendorChunk in webpack 5 #6822

Closed
wants to merge 6 commits into from
Closed

fix(web): fix styles vendorChunk in webpack 5 #6822

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1a5b78b
fix(web): fix styles vendorChunk in webpack 5
markogresak Aug 23, 2021
283fcc2
cleanup(web): use same test function for webpack 4 and 5
markogresak Aug 23, 2021
eaa6394
cleanup(web): use exact webpack version range
markogresak Aug 23, 2021
3b129c9
cleanup(web): remove webpack-5 types, use stub types for now
markogresak Sep 3, 2021
10a3901
Merge remote-tracking branch 'upstream/master' into fix-styles-bundle
markogresak Sep 3, 2021
e08b534
chore(web): remove change to package.json
markogresak Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -77,8 +77,8 @@
"@pmmmwh/react-refresh-webpack-plugin": "^0.4.3",
"@popperjs/core": "^2.9.2",
"@reduxjs/toolkit": "1.5.0",
"@rollup/plugin-commonjs": "^20.0.0",
"@rollup/plugin-babel": "^5.3.0",
"@rollup/plugin-commonjs": "^20.0.0",
"@rollup/plugin-image": "^2.1.0",
"@rollup/plugin-json": "^4.1.0",
"@rollup/plugin-node-resolve": "^13.0.4",
Expand Down Expand Up @@ -239,6 +239,7 @@
"url-loader": "^3.0.0",
"verdaccio": "^5.0.4",
"webpack": "4.46.0",
"webpack-5": "npm:webpack@^5.51.1",
"webpack-dev-server": "3.11.2",
"webpack-merge": "4.2.1",
"webpack-node-externals": "1.7.2",
Expand Down
42 changes: 23 additions & 19 deletions packages/web/src/utils/third-party/cli-files/models/webpack-configs/browser.ts
View file
Open in desktop
Expand Up @@ -5,6 +5,8 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
// TODO: import from 'webpack' in Nx 13
import type { Module, ModuleGraph, ChunkGraph, Chunk } from 'webpack-5';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not want to introduce problems by changing to webpack@5, so I am adding this alias as

yarn add -D 'webpack-5@npm:webpack@^5.51.1'

Copy link
Member

Choose a reason for hiding this comment

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

I think this would cause a problem if the user does not want webpack 5 installed. For now I think it's better to leave things untyped until Nx 13 when we remove webpack 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaysoo it's just a type for now (@types/webpack), would that be a problem too?

I have pushed 3b129c9 to remove the dependency and changed to stub types. This should make it easier to swtich the types for Nx 13.

import { LicenseWebpackPlugin } from 'license-webpack-plugin';
import { WebpackConfigOptions, BuildOptions } from '../build-options';
import {
Expand Down Expand Up @@ -120,30 +122,32 @@ export function getBrowserConfig(wco: WebpackConfigOptions) {
}
: {}),
vendors: false,
// TODO(jack): Support both 4 and 5
vendor: !!buildOptions.vendorChunk && {
name: 'vendor',
chunks: isWebpack5 ? (chunk) => chunk.name === 'main' : 'initial',
enforce: true,
test: isWebpack5
? /[\\/]node_modules[\\/]/
: (
module: { nameForCondition?: Function },
chunks: Array<{ name: string }>
) => {
const moduleName = module.nameForCondition
? module.nameForCondition()
: '';
test: (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test logic is the same, the only difference for isWebpack5 is the chunks condition. If preferred, we can revert the commit 283fcc2 to have separate function for isWebpack5.

module: Module,
// `CacheGroupsContext`, but it's not exported from webpack types
context: { moduleGraph: ModuleGraph; chunkGraph: ChunkGraph }
) => {
const moduleName = module.nameForCondition
? module.nameForCondition()
: '';
// TODO(jack): Remove in Nx 13
const chunks = isWebpack5
? context.chunkGraph.getModuleChunks(module)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going for the same condition as the webpack 4.
I'm not very familiar with the webpack API, so I am not sure if there is a better way to achieve this. I found that there's the chunksFilter config option, but it's called only with the chunk value, so I'm not sure if it would work.

: (context as unknown as Chunk[]);

return (
/[\\/]node_modules[\\/]/.test(moduleName) &&
!chunks.some(
({ name }) =>
isPolyfillsEntry(name) ||
globalStylesBundleNames.includes(name)
)
);
},
return (
/[\\/]node_modules[\\/]/.test(moduleName) &&
!chunks.some(
({ name }) =>
isPolyfillsEntry(name) ||
globalStylesBundleNames.includes(name)
)
);
},
},
},
},
Expand Down