Webpack + VFS = <3 #525

hansl · 2018-03-18T09:12:20Z

No description provided.

filipesilva · 2018-03-18T17:42:29Z

packages/angular_devkit/build_webpack/src/browser/index.ts

+        // TODO: fix webpack typings.
+        // tslint:disable-next-line:no-any
+        (webpackCompiler as any).inputFileSystem = new WebpackFileSystemHostAdapter(
+          new NodeJsSyncHost(),


Shouldn't this be using the this.context.host instead of a new one? Or at least one based on the that one.

Yes it should. Sorry I'll fix it. I tried it and it was failing, so decided to focus on one error at a time. Since all paths are absolute not to the Host but to the file system, that's a deeper change in general.

filipesilva · 2018-03-18T17:44:31Z

packages/angular_devkit/build_webpack/src/utils/webpack-file-system-host-adapter.ts

+
+  readFile(path: string, callback: Callback<Buffer>): void {
+    const o = this._host.read(normalize('/' + path)).pipe(
+      map(content => new Buffer(content)),


This should be mapped to string via virtualFs.fileBufferToString.

Actually the webpack adapter wants to have node Buffers as a return, because it expects this to match the fs interface.

Oh, I didn't know that. In the ngtools/webpack FS we return the string. Maybe that's broken there.

It's not broken for source files. The only thing Webpack is doing is toString('utf-8'), which works for strings 🤷

filipesilva · 2018-03-18T17:44:59Z

packages/angular_devkit/build_webpack/src/utils/webpack-file-system-host-adapter.ts

+  }
+
+  readlink(path: string, callback: Callback<string>): void {
+    const err: NodeJS.ErrnoException = new Error('Not a symlink.');


We should support symlinks in the future.

I disagree, at least as far as the host is concerned, symlinks were intentionally left out of the design.

We "support" them by having the Node*Host supports proper stats and you check them, but if you don't know which host comes in then you cannot know (and shouldn't IMO).

Ok, I don't have any usecase that needs them right now either.

filipesilva · 2018-03-18T17:47:44Z

packages/angular_devkit/build_webpack/src/utils/webpack-file-system-host-adapter.ts

+  }
+
+  readdir(path: string, callback: Callback<string[]>): void {
+    return this._doHostCall(this._host.list(normalize('/' + path)), callback);


I think this will need to return the system paths instead of the normalized Paths.

Maybe as input, but readdir returns fragments which do not contain slashes.

Good point.

filipesilva · 2018-03-18T17:48:54Z

packages/angular_devkit/build_webpack/src/utils/webpack-file-system-host-adapter.ts

+    throw err;
+  }
+
+  purge(_changes?: string[] | string): void {}


This will need to be implemented in the future.

I'm not sure what this should do for our own adapter. For a cache system you can clear the cache, but here I don't thikn there's anything to do. purge is optional on the real typings btw.

Let's leave it off and see if there's any problem then.

clydin · 2018-03-20T22:02:44Z

packages/angular_devkit/core/src/virtual-fs/host/memory.ts

-  stat(_path: Path): Observable<Stats<{}>> | null {
-    return null;
+  // Some hosts may not support stat.
+  stat(path: Path): Observable<Stats<{}>> {


Observable<Stats>?

clydin · 2018-03-20T22:04:13Z

packages/angular_devkit/core/src/virtual-fs/host/memory.ts


-    return observableOf(this._cache.has(path) || this._isDir(path));
+  list(path: Path): Observable<PathFragment[]> {


Thoughts on this returning Observable<PathFragment> instead of the array. Would allow the consumer to operator on the stream of elements directly.

I see Observable<X> here as a replacement of Promise<>. The contract is that these will always emit once and complete, or error out. I like the consistence of this interface too.

clydin · 2018-03-20T22:10:40Z

packages/angular_devkit/core/node/host.ts

+    return new Observable(obs => {
+      // Create folders if necessary.
+      const _createDir = (path: Path) => {
+        if (fs.existsSync(getSystemPath(path))) {


Probably a refactor for the future but for all the sync calls could we use the async versions with utils.promisify and convert and return the observabled promise?

You mean the async ones? The sync ones need to be sync.

Yes. Sorry for the confusion.

clydin · 2018-03-20T22:19:28Z

packages/angular_devkit/build_webpack/src/browser/index.ts

+
+        // TODO: fix webpack typings.
+        // tslint:disable-next-line:no-any
+        (webpackCompiler as any).inputFileSystem = new WebpackFileSystemHostAdapter(


The watch file system will be unaffected and I don't think caching will be used. This webpack plugin is always applied before anything else: https://github.com/webpack/webpack/blob/master/lib/node/NodeEnvironmentPlugin.js
https://github.com/webpack/webpack/blob/master/lib/webpack.js#L34

clydin · 2018-03-20T22:34:37Z

packages/ngtools/webpack/src/gather_diagnostics.ts

@@ -56,29 +56,29 @@ export function gatherDiagnostics(
    const tsProgram = program as ts.Program;
    // Check syntactic diagnostics.
    time(`${benchmarkLabel}.gatherDiagnostics.ts.getSyntacticDiagnostics`);
-    checkDiagnostics(tsProgram.getSyntacticDiagnostics);
+    checkDiagnostics(tsProgram.getSyntacticDiagnostics.bind(tsProgram));


() => tsProgram.getSyntacticDiagnostics() ?

Yeah that looks better imho.

clydin · 2018-03-20T22:43:09Z

packages/ngtools/webpack/src/angular_compiler_plugin.ts

@@ -87,6 +88,8 @@ export interface AngularCompilerPluginOptions {

  // Use tsconfig to include path globs.
  compilerOptions?: ts.CompilerOptions;
+
+  host: virtualFs.Host<fs.Stats>;


…ble rights

…which is more correct And fits with the rest of the fs methods.

… observables

…le system

filipesilva · 2018-03-21T12:48:47Z

packages/angular_devkit/build_webpack/test/utils/run-target-spec.ts

@@ -31,7 +32,7 @@ export function runTargetSpec(
  host: TestProjectHost,
  targetSpec: TargetSpecifier,
  overrides = {},
-  logger: logging.Logger = new logging.NullLogger(),
+  logger: logging.Logger = createConsoleLogger(),


This change should be reverted.

filipesilva · 2018-03-21T13:03:44Z

I will followup this PR with fixes for the Windows CI.

googlebot added the cla: yes label Mar 18, 2018

hansl force-pushed the vfs-webpack branch 2 times, most recently from e347e78 to a39684c Compare March 18, 2018 17:33

filipesilva suggested changes Mar 18, 2018

View reviewed changes

hansl force-pushed the vfs-webpack branch 10 times, most recently from 07a9203 to c796862 Compare March 20, 2018 04:53

clydin suggested changes Mar 20, 2018

View reviewed changes

filipesilva previously approved these changes Mar 20, 2018

View reviewed changes

hansl added 14 commits March 20, 2018 18:12

fix(@angular-devkit/architect-cli): architect CLI should have executa…

15d59f1

…ble rights

fix(@angular-devkit/core): rename Host.stats to Host.stat (singular) …

f4d357d

…which is more correct And fits with the rest of the fs methods.

fix(@angular-devkit/core): default for Stats<> generic

0b00b58

fix(@angular-devkit/core): add stats to memory host

8dcc900

feat(@angular-devkit/core): streamline sync host with regular host

1aa9e6a

feat(@angular-devkit/core): NodeJsSyncHost should behave properly WRT…

25a8b08

… observables

feat(@angular-devkit/build-webpack): use a Host for webpacks input fi…

e27e966

…le system

ci: untie tsconfig a bit, but tighten tslint to cover

12a83b5

feat(@ngtools/webpack): move the webpack plugin to devkit repo

70bc3d7

build: fix all lint errors for @ngtools/webpack

0bcb670

build: fix typescript errors for @ngtools/webpack

bf16da3

build: remove package.json dependency on @ngtools/webpack

40a52fb

fix(@ngtools/webpack): fix ngtools/webpack to properly use Host<>

e9ccc62

test: fix glob option in tests

76d47cb

hansl dismissed filipesilva’s stale review via c880b24 March 21, 2018 01:14

hansl force-pushed the vfs-webpack branch 2 times, most recently from c880b24 to 0bcf4f8 Compare March 21, 2018 04:47

hansl added 2 commits March 20, 2018 22:08

test: fix large tests for webpack VFS support

e0c2d20

test: fix ngtools/webpack local-bootstrap resolution

681f669

hansl force-pushed the vfs-webpack branch from 0bcf4f8 to 681f669 Compare March 21, 2018 05:08

clydin approved these changes Mar 21, 2018

View reviewed changes

filipesilva reviewed Mar 21, 2018

View reviewed changes

filipesilva approved these changes Mar 21, 2018

View reviewed changes

filipesilva merged commit d7e7b95 into angular:master Mar 21, 2018

filipesilva mentioned this pull request Mar 21, 2018

VFS followup #542

Merged

filipesilva mentioned this pull request May 15, 2018

Support mutation testing with angular cli angular/angular-cli#8744

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webpack + VFS = <3 #525

Webpack + VFS = <3 #525

hansl commented Mar 18, 2018

filipesilva Mar 18, 2018

hansl Mar 18, 2018

filipesilva Mar 18, 2018

hansl Mar 18, 2018

filipesilva Mar 18, 2018

hansl Mar 18, 2018

filipesilva Mar 18, 2018

hansl Mar 18, 2018

filipesilva Mar 18, 2018

filipesilva Mar 18, 2018

hansl Mar 18, 2018

filipesilva Mar 18, 2018

filipesilva Mar 18, 2018

hansl Mar 18, 2018

filipesilva Mar 18, 2018

clydin Mar 20, 2018

clydin Mar 20, 2018

hansl Mar 20, 2018

clydin Mar 20, 2018

hansl Mar 20, 2018

clydin Mar 21, 2018

clydin Mar 20, 2018

clydin Mar 20, 2018

filipesilva Mar 20, 2018

clydin Mar 20, 2018

filipesilva Mar 21, 2018

filipesilva commented Mar 21, 2018


		return observableOf(this._cache.has(path) \|\| this._isDir(path));
		list(path: Path): Observable<PathFragment[]> {

Webpack + VFS = <3 #525

Webpack + VFS = <3 #525

Conversation

hansl commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipesilva commented Mar 21, 2018