Skip to content

updating webpack config for differential loading #14015

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
/**
Copy link
Collaborator

@alan-agius4 alan-agius4 Apr 1, 2019

Choose a reason for hiding this comment

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

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.

* @license
* Copyright Google Inc. All Rights Reserved.
*
* 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
*/


// This updates a webpack configuration which has been created by a
// webpack-based builder for differential loading. It duplicates an
// existing one 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 to use.
//
// see: https://docs.google.com/document/d/13k84oGwrEjwPyAiAjUgaaM7YHJrzYXz7Cbt6CwRp9N4/


import { AngularCompilerPlugin } from '@ngtools/webpack';
import * as webpack from 'webpack';

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


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

Choose a reason for hiding this comment

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

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

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?


const legacyConfig: webpack.Configuration = {
...config,
output: buildLegacyOutput(config),
plugins: buildLegacyPlugins(config),
resolve: buildLegacyResolve(config),
};

const modernConfig: webpack.Configuration = {
Copy link
Collaborator

@alan-agius4 alan-agius4 Apr 1, 2019

Choose a reason for hiding this comment

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

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

...config,
output: buildModernOutput(config),
plugins: buildModernPlugins(config),
entry: buildModernEntry(config),
resolve: buildModernResolve(config),
};

const newConfig = [legacyConfig, modernConfig];

return newConfig;
}

function buildLegacyOutput(config: webpack.Configuration): webpack.Output | undefined {

if (!config.output || !config.output.filename) {
return config.output;
}

const filename = config.output.filename.replace('[name]', '[name].legacy');

return { ...config.output, filename };
}

function buildModernOutput(config: webpack.Configuration): webpack.Output | undefined {

if (!config.output || !config.output.filename) {
return config.output;
}

const filename = config.output.filename.replace('[name]', '[name].modern');

return { ...config.output, filename };
}

function buildModernResolve(config: webpack.Configuration): webpack.Resolve | undefined {

if (!config.resolve || !config.resolve.mainFields) {
return config.resolve;
}

const modernResolve = { ...config.resolve };
modernResolve.mainFields = ['es2015', 'module', 'browser', 'main'];

return modernResolve;
}

function buildLegacyResolve(config: webpack.Configuration): webpack.Resolve | undefined {

if (!config.resolve || !config.resolve.mainFields) {
return config.resolve;
}

const modernResolve = { ...config.resolve };
modernResolve.mainFields = ['module', 'browser', 'main'];

return modernResolve;
}

function buildModernEntry(config: webpack.Configuration): webpack.Entry | undefined {
const entry = config.entry;

if (!entry) {
return undefined;
}

const polyfillsEntry = entry as webpack.Entry;

const polyfills: string[] = polyfillsEntry['polyfills'] as string[];

if (!polyfills) {
return { ...polyfillsEntry };
}

const tweakPolyfills = (file: string) => {

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

if (file.endsWith('polyfills.ts')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return file.replace('polyfills.ts', 'polyfills.modern.ts');
} else {
return file;
}
};

const modernPolyfills = polyfills.map(tweakPolyfills);
const modernEntry = { ...entry as webpack.Entry, polyfills: modernPolyfills };

return modernEntry;
}

function buildLegacyPlugins(config: webpack.Configuration): webpack.Plugin[] | undefined {
Copy link
Collaborator

@alan-agius4 alan-agius4 Apr 1, 2019

Choose a reason for hiding this comment

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

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.

const plugins = config.plugins;

if (!plugins) {
return plugins;
}

const legacyPlugins = [...plugins];

const acpIndex = plugins.findIndex(p => p.constructor.name === 'AngularCompilerPlugin');
const acp = plugins[acpIndex] as AngularCompilerPlugin;
const legacyAcp = buildLegacyAngularCompilerPlugin(acp);
legacyPlugins[acpIndex] = legacyAcp;

return legacyPlugins;
}

function buildModernPlugins(config: webpack.Configuration): webpack.Plugin[] {
const plugins = config.plugins;

if (!plugins) {
return [];
}

const modernPlugins = [...plugins];

const statsPluginIndex = plugins.findIndex(p => p.constructor.name === 'StatsPlugin');
if (statsPluginIndex > -1) {
const modernStatsPlugin = buildModernStatsPlugin();
modernPlugins[statsPluginIndex] = modernStatsPlugin;
}

return modernPlugins;
}

function buildModernStatsPlugin(): webpack.Plugin {
Copy link
Collaborator

@alan-agius4 alan-agius4 Apr 1, 2019

Choose a reason for hiding this comment

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

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?

const modernStatsPlugin = new StatsPlugin();
modernStatsPlugin.output = 'stats.modern.json';

return modernStatsPlugin;
}

function buildLegacyAngularCompilerPlugin(acp: AngularCompilerPlugin): AngularCompilerPlugin {
const options = acp.options;
const modernOptions = { ...options };

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

let tsConfigPath = modernOptions.tsConfigPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

  1. tsconfig for the app es5
  2. tsconfig for the app es2015
  3. tsconfig for the worker es5
  4. tsconfig for the worker es2015

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@alan-agius4 alan-agius4 Apr 1, 2019

Choose a reason for hiding this comment

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

You can pass the compiler options to the AngularCompilerPlugin.

export interface AngularCompilerPluginOptions {

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

if (tsConfigPath.endsWith('.json')) {
tsConfigPath = tsConfigPath.substr(0, tsConfigPath.length - 5);
}
tsConfigPath += '.legacy.json';

modernOptions.tsConfigPath = tsConfigPath;

const legacyAcp = new AngularCompilerPlugin(modernOptions);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@alan-agius4 alan-agius4 Apr 1, 2019

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


return legacyAcp;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* 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
*/

import { AngularCompilerPlugin } from '@ngtools/webpack';
import * as path from 'path';
import {
createWebpackMultiConfig,
} from '../../src/differential-loading/create-webpack-multi-config';

const devkitRoot = (global as any)._DevKitRoot; // tslint:disable-line:no-any
const workspaceRoot = path.join(
devkitRoot,
'tests/angular_devkit/build_angular/test-differential-loading/');

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

const tsConfig = path.join(workspaceRoot, 'tsconfig.test.json');

describe('differential loading webpack config', () => {

it('will be updated', () => {

const config = {
output: {
filename: '[name].js',
},
resolve: {
mainFields: ['main'],
},
entry: {
'main': 'main.js',
},
plugins: [
new AngularCompilerPlugin({tsConfigPath: tsConfig }),
],
};

const newConfig = createWebpackMultiConfig(config);

expect(Array.isArray(newConfig)).toBe(true);
expect(newConfig.length).toBe(2);
});
});
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"compileOnSave": false,
"compilerOptions": {
"baseUrl": "./",
"outDir": "./dist/out-tsc",
"sourceMap": true,
"declaration": false,
"moduleResolution": "node",
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"target": "es2015",
"module": "esnext",
"typeRoots": [
"node_modules/@types"
],
"lib": [
"es2017",
"dom"
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"compileOnSave": false,
"compilerOptions": {
"baseUrl": "./",
"outDir": "./dist/out-tsc",
"sourceMap": true,
"declaration": false,
"moduleResolution": "node",
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"target": "es5",
"module": "esnext",
"typeRoots": [
"node_modules/@types"
],
"lib": [
"es2017",
"dom"
]
}
}