updating webpack config for differential loading #14015

manfredsteyer · 2019-03-28T18:24:23Z

No description provided.

alexeagle

the commit description is not nearly descriptive enough. "tweak" could be anything. Please explain the difference in behavior (and also probably state how this is used or not used yet)

see AlexR's preso to the team about commit messages from a couple months back

alexeagle · 2019-03-29T17:12:10Z

packages/angular_devkit/build_angular/src/differential-loading/update-webpack-config.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+type WebpackConfig = any; // tslint:disable-line


why? either give a local type that matches the shape, or import a correct type, or explain in the //tslint comment why you can't

alexeagle · 2019-03-29T17:12:59Z

packages/angular_devkit/build_angular/src/differential-loading/update-webpack-config.ts

+type WebpackConfig = any; // tslint:disable-line
+type WebpackPlugin = any; // tslint:disable-line
+
+export function updateWebpackConfig(config: WebpackConfig): WebpackConfig {


"update" is not descriptive. could you find a name that says what is the update?

createSubConfigs or something? (what does webpack call it when configs are nested in this way?)

alexeagle · 2019-03-29T17:13:46Z

packages/angular_devkit/build_angular/src/differential-loading/update-webpack-config.ts

+
+  const tweakPolyfills = (file: string) => {
+
+    // TODO: Do we need an own polyfill file for modern browsers? If yes, where is it created?


when do we plan to fix the TODO? can we figure it out in this PR?

Question for reviewers: Do we need an own polyfill file for modern browsers? If yes, where is it created? (e. g. by a schematic or by this logic on the fly?)

alexeagle · 2019-03-29T17:14:26Z

packages/angular_devkit/build_angular/src/differential-loading/update-webpack-config.ts

+ *
+ * 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
+ */


needs some file-level commenting explaining what's going on, in case the reader hasn't seen the design doc. linking to the design might also be nice

alexeagle · 2019-03-29T17:14:52Z

packages/angular_devkit/build_angular/src/differential-loading/update-webpack-config.ts

+  return modernStatsPlugin;
+}
+
+function buildLegacyAcp(acp: WebpackPlugin): object {


Acp is that intentional spelling?

alexeagle · 2019-03-29T17:15:03Z

packages/angular_devkit/build_angular/src/differential-loading/update-webpack-config.ts

+  const options = acp._options;
+  const modernOptions = { ...options };
+
+  // TODO: Where shall we create this tsconfig with target=es5?


will we address the TODO in this PR?

Question for reviewers: Where shall we create this tsconfig with target=es5? Schematics? On the fly if it does not exist?

…erential loading This updates a webpack configuration which has been created by a webpack-based builder for differential loading. For this, it duplicates the existing configuration and modifies e. g. the name of the bundles to generate (` [name].modern.js` instead of `[name].js`), the resolve array, and the Angular Compiler Plugin’s reference to the tsconfig.json. Because of this strategy, just one webpack config needs to be maintained per builder. The other one is retrieved from it by this logic. This is esp. important if the CLI supports several bundles in future, e. g. ES5, ES2015, ES2016, ES2017, etc. This PR is currently not used but it is the foundation for further ones. Also see: https://docs.google.com/document/d/13k84oGwrEjwPyAiAjUgaaM7YHJrzYXz7Cbt6CwRp9N4/edit?ts=5c652052#heading=h.gijrsdjw51q5

alan-agius4

See above.

alan-agius4 · 2019-03-31T09:43:43Z

packages/angular_devkit/build_angular/test/differential-loading/webpack_config_spec.ts

+const workspaceRoot = path.join(
+  devkitRoot,
+  'tests/angular_devkit/build_angular/test-differential-loading/');
+


Try to avoid using the files from the file system directly, to test ideally always use the abstracted fs, as it would be easier to update a property in a file, write another file etc.. during a specs.

See here for an example: https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_webpack/src/webpack-dev-server/index_spec_large.ts

#13993 (comment)

alan-agius4 · 2019-03-31T09:52:12Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

+
+  modernOptions.tsConfigPath = tsConfigPath;
+
+  const legacyAcp = new AngularCompilerPlugin(modernOptions);


Has this been tested? Afaik this plugin doesn’t support multicompiler or we won’t use multi webpack compiler at all and use two separate webpack instances?

In addition a single build can have multiple AngularCompilerPlugin when using Web Workers, which is not being handled here.

Yes. It works if we adopt some methods a bit.

Just to confirm are you going to go for webpack multicompiler?

Also can you elaborate what you mean by adopt some methods a bit?

At the moment @angular-devkit/build-webpack doesn’t have implementations for multi-compiler.

alan-agius4 · 2019-03-31T09:54:01Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

+  // TODO: Where shall we create this tsconfig with target=es5?
+  //  Schematics? On the fly if it does not exist?
+
+  let tsConfigPath = modernOptions.tsConfigPath;


I think creating an extra config, might complicate things a bit especially considering the web workers, which need a separate tsconfig, meaning that each 'build' will have multiple config files. See: #13700

Not sure if it best to change the file script target on the fly.

//cc @filipesilva what do you think?

Also this would increase maintain in tsconfig files for users.

So, if users have workers and a differential severing they end up with 4 tsconfig for their application which users would need to keep in sync if they do a change.

tsconfig for the app es5

tsconfig for the app es2015

tsconfig for the worker es5

tsconfig for the worker es2015

TBH, I don't like it either. However, I see no alternative for this. Or is it possible to pass an object literal representing the contents of a tsconfig.json to the AngularCompilerPlugin? I guess, in this case the AngularCompilerPlugin also needs to write it to hard disk.

You can pass the compiler options to the AngularCompilerPlugin.

angular-cli/packages/ngtools/webpack/src/interfaces.ts

Line 27 in 42ef582

export interface AngularCompilerPluginOptions {

Why should the angular compiler plugin write the file to disk?

Ah, that means I could read the tsconfig.json into an object, modify it's target in memory and assign it to the compilerOptions property, right? In this case we don't need a seperate tsconfig.json for this target on disk. Do I see this right?

alan-agius4 · 2019-03-31T10:05:33Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

+
+    // TODO: Do we need an own polyfill file for modern browsers? If yes, where is it created?
+
+    if (file.endsWith('polyfills.ts')) {


This doesn’t seem right, polyfills file can be named anything. It can be foo.ts.

https://angular.io/cli/build

Also, I believe polyfills for modern and legacy will be internal which @clydin is working on something for zone.js

alan-agius4 · 2019-04-01T06:58:22Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

+const StatsPlugin = require('stats-webpack-plugin');
+
+
+export function createWebpackMultiConfig(config: webpack.Configuration): webpack.Configuration[] {


I am not quite understanding how this fits in the builder. You need a webpack config here and you transform it afterwards.

Why not use the existing methods to generate two separate config files and avoid post transformations?

Example:

const webpackConfigs = (wco) => [ getCommonConfig(wco), getBrowserConfig(wco), getStylesConfig(wco), getStatsConfig(wco), ]; const [legacyConfig, modernConfig] = [ generateBrowserWebpackConfigFromContext( legacyBrowserOptions, context, wco => webpackConfigs(wco), host, ), generateBrowserWebpackConfigFromContext( moderBrowserOptions, context, wco => webpackConfigs(wco), host, ), ];

Then, based on the type of build type change the file output paths and script target, This is something similar to how the serve builder generates it's webpack config. You can also use the new transformations hooks, if you need to do post transformation. Something like https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/dev-server/index.ts#L123 //@clydin knows more about this feature.

There are some helper methods for webpack here: https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/utils/webpack-browser-config.ts

However, I am still not sure how everything will be glued together seeing this logic. As based on the design doc I was under the impression that the browser builder will implement the differential serving logic and the differential-builder responsibility was more to orchestrate to the different browser builders.

If the browser builder will be responsible for both builds, should this code

angular-cli/packages/angular_devkit/build_angular/src/browser/index.ts

Lines 267 to 277 in 42ef582

switchMap(() => from(buildBrowserWebpackConfigFromContext(options, context, host))),

switchMap(({ workspace, config }) => {

if (configFn) {

return configFn(workspace, config).pipe(

map(config => ({ workspace, config })),

);

} else {

return of({ workspace, config });

}

}),

switchMap(({workspace, config}) => {

need to change and run buildBrowserWebpackConfigFromContext run twice?

alan-agius4 · 2019-04-01T10:23:40Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

+  return modernPlugins;
+}
+
+function buildModernStatsPlugin(): webpack.Plugin {


What about the other profile plugins such as the chrome-profiler and speed-measure and also the license plugin https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/browser.ts#L64?

alan-agius4 · 2019-04-01T12:13:25Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

@@ -0,0 +1,185 @@
+/**


inline comments near the implementations would definitely help the reviewers and future engineers that go through the code as they wouldn't need to jump from an implementation to another to understand why something is needed.

alan-agius4 · 2019-04-01T12:15:36Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

+  return modernEntry;
+}
+
+function buildLegacyPlugins(config: webpack.Configuration): webpack.Plugin[] | undefined {


Why not generate a webpack config which is already property configured for an es2015 or es5 build? Rather than having to go through certain plugins and re-create them, with a risk of skipping one and increasing maintenance, if the original webpack configurations are changed.

alan-agius4 · 2019-04-01T12:43:05Z

packages/angular_devkit/build_angular/src/differential-loading/create-webpack-multi-config.ts

+    resolve: buildLegacyResolve(config),
+  };
+
+  const modernConfig: webpack.Configuration = {


With this approach you are missing some configuration for es2015 https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts#L236 and https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts#L216

Hence, I strongly recommend that we avoid doing such transformations and let the webpack config builder return the appropriate configs as this is quite brittle

angular-automatic-lock-bot · 2019-09-13T00:41:37Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Mar 28, 2019

alexeagle reviewed Mar 29, 2019

View reviewed changes

alexeagle requested a review from alan-agius4 March 29, 2019 17:16

filipesilva requested a review from clydin March 29, 2019 19:47

manfredsteyer changed the title ~~tweak webpack config for differential loading~~ updating webpack config for differential loading Mar 30, 2019

alan-agius4 requested changes Apr 1, 2019

View reviewed changes

alan-agius4 reviewed Apr 1, 2019

View reviewed changes

manfredsteyer closed this Apr 1, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019


		const tweakPolyfills = (file: string) => {

		// TODO: Do we need an own polyfill file for modern browsers? If yes, where is it created?


		modernOptions.tsConfigPath = tsConfigPath;

		const legacyAcp = new AngularCompilerPlugin(modernOptions);


		// TODO: Do we need an own polyfill file for modern browsers? If yes, where is it created?

		if (file.endsWith('polyfills.ts')) {

		const StatsPlugin = require('stats-webpack-plugin');


		export function createWebpackMultiConfig(config: webpack.Configuration): webpack.Configuration[] {

	switchMap(() => from(buildBrowserWebpackConfigFromContext(options, context, host))),
	switchMap(({ workspace, config }) => {
	if (configFn) {
	return configFn(workspace, config).pipe(
	map(config => ({ workspace, config })),
	);
	} else {
	return of({ workspace, config });
	}
	}),
	switchMap(({workspace, config}) => {

updating webpack config for differential loading #14015

updating webpack config for differential loading #14015

Uh oh!

Conversation

manfredsteyer commented Mar 28, 2019

Uh oh!

alexeagle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manfredsteyer Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 13, 2019

Uh oh!

Uh oh!

manfredsteyer Mar 29, 2019 •

edited

Loading

alan-agius4 Apr 1, 2019 •

edited

Loading

alan-agius4 Apr 1, 2019 •

edited

Loading

alan-agius4 Apr 1, 2019 •

edited

Loading

alan-agius4 Apr 1, 2019 •

edited

Loading

alan-agius4 Apr 1, 2019 •

edited

Loading

alan-agius4 Apr 1, 2019 •

edited

Loading