feat: add support for `watch` when building a library #11358

alan-agius4 · 2018-06-24T13:00:09Z

add support for watch when building a library

ng-packagr version 4.0.0-rc.2, lands the incremental builds feature.

More info: https://github.com/dherges/ng-packagr/blob/master/CHANGELOG.md#400-rc2-2018-06-23

BREAKING CHANGE:

ng-packagr version 4.0.0-rc.2 or greater is required.
enableResourceInlining needs to be enabled for libraries that contain components

Ps: most likely this should go in V7, but still felt like doing it, so to be ready.

Closes: #11100

filipesilva · 2018-07-02T00:59:20Z

packages/angular_devkit/build_ng_packagr/package.json

@@ -14,6 +14,6 @@
    "rxjs": "^6.0.0"
  },
  "peerDependencies": {
-    "ng-packagr": "^2.2.0 || ^3.0.0 || ^4.0.0-rc.0"


I don't think we can drop support for 2.x and 3.x here, since that would be a breaking change for us. WDYT of having the watch option verify for a compatible version of ng-packagr instead?

We've had a couple of options that work like that in the past. Current examples include

angular-cli/packages/angular_devkit/build_angular/src/angular-cli-files/utilities/service-worker/index.ts

Lines 91 to 97 in 6449a75

if (!semver.gte(swVersion, NEW_SW_VERSION)) {

throw new Error(tags.stripIndent`

The installed version of @angular/service-worker is ${swVersion}. This version of the CLI

requires the @angular/service-worker version to satisfy ${NEW_SW_VERSION}. Please upgrade

your service worker version.

`);

}

and

angular-cli/packages/angular/cli/commands/serve.ts

Lines 27 to 28 in 6449a75

Version.assertCompatibleAngularVersion(this.project.root);

Version.assertTypescriptVersion(this.project.root);

I like the whole idea!

filipesilva · 2018-07-02T02:35:04Z

packages/schematics/angular/library/files/__projectRoot__/tsconfig.lib.json

@@ -24,7 +24,8 @@
    "fullTemplateTypeCheck": true,
    "strictInjectionParameters": true,
    "flatModuleId": "AUTOGENERATED",
-    "flatModuleOutFile": "AUTOGENERATED"
+    "flatModuleOutFile": "AUTOGENERATED",
+    "enableResourceInlining": true


Is this required for the watch mode? If so we might need to warn users of older versions about it.

Yes, this is required for V4. As we removed a lot of internal logic to inline resources.

Ok, then I think we'll need to check this as well when asserting compatibility.

For v3, this flag might cause some unwanted behaviour I think.

Yeah makes sense.

Maybe we can fully remove these values and let ng-packagr take care of them? I think we ended up adding them manually to get around a tsconfig merging bug in ng-packagr. It might not be present anymore.

If they are still needed then we need to do the health check both ways then: that they are not present when using ng-packagr v3, and that they are when using v4.

Actually I gave this a bit more thought. And I think. IN V3, this might not have any sideEffects as there is a previous step to transforms the components.

I have updated the v3 branch to include this flag, and from the Unit Tests all seem fine
ng-packagr/ng-packagr#989

filipesilva · 2018-07-02T02:45:20Z

packages/schematics/angular/library/files/__projectRoot__/ng-package.json

@@ -1,7 +1,6 @@
 {
  "$schema": "<%= relativePathToWorkspaceRoot %>/node_modules/ng-packagr/ng-package.schema.json",
  "dest": "<%= relativePathToWorkspaceRoot %>/<%= distRoot %>",
-  "deleteDestPath": false,


I'd like to understand the motivation to remove this preset better. Does this option not play well with watch mode?

I originally added it to enable users to re-run build in dev mode without breaking watched files. But now with watch mode existing, this usage might be different.

I guess ideally we'd want a single ng-package.json file at all time, with the destination path being automatically deleted in single-run mode and not in watch mode.

But that does beg the question: if deleteDestPath does not interact well with watch mode, maybe it should be ignored in ng-packagr directly.

Regardless of the final semantics of watch mode, I do agree on doing away with the dev/prod configs if their only difference is watch mode support.

It’s not the it doesn’t play well. It’s that with watch mode it”s kinda redundant.

The reason behing the original implementation of deleteDestPath was for users to create their own watch, and due to the fact that in webpack 3 when a file got deleted from disk webpack would crash with a fatal error. Now this is super seeded with the watch mode.

It's also important to remove the dist dir overall though. If you don't, artifacts that aren't there in subsequent builds will still be present because of previous builds.

With apps this is especially relevant because of assets, but less relevant for libraries. Still, the correct thing to do on a fresh, non-watch build is to clean (delete) the outpath before re-generating artifacts.

That was why we had it on on the production builds, and not the development ones. The production builds were the 'correct' ones, while the dev builds made that concession for a makeshift watch mode.

By default ng-packagr deletes the dist directory when when triggering the build/watch

Ah I wasn't aware of that! Then removing the option makes perfect sense.

alan-agius4 · 2018-07-02T06:53:23Z

@filipesilva I updated the PR as suggested.

Do you have any sample on how should I test the watch?

alan-agius4 · 2018-07-02T07:21:00Z

AppVeyor is failing due to the recurring issue Error: ENOTEMPTY: directory not empty, rmdir

filipesilva · 2018-07-02T11:21:12Z

A good way to test the watch mode is to both check the build events are being emitted properly, and that the build artifacts contain what you expect.

A good example can be found here

angular-cli/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts

Lines 23 to 112 in 6449a75

    
             it('rebuilds on TS file changes', (done) => { 
        
               const goldenValueFiles: { [path: string]: string } = { 
        
                 'src/app/app.module.ts': ` 
        
                   import { BrowserModule } from '@angular/platform-browser'; 
        
                   import { NgModule } from '@angular/core'; 
        
                   import { AppComponent } from './app.component'; 
        
                   @NgModule({ 
        
                     declarations: [ 
        
                       AppComponent 
        
                     ], 
        
                     imports: [ 
        
                       BrowserModule 
        
                     ], 
        
                     providers: [], 
        
                     bootstrap: [AppComponent] 
        
                   }) 
        
                   export class AppModule { } 
        
                   console.log('$$_E2E_GOLDEN_VALUE_1'); 
        
                   export let X = '$$_E2E_GOLDEN_VALUE_2'; 
        
                 `, 
        
                 'src/main.ts': ` 
        
                   import { enableProdMode } from '@angular/core'; 
        
                   import { platformBrowserDynamic } from '@angular/platform-browser-dynamic'; 
        
                   import { AppModule } from './app/app.module'; 
        
                   import { environment } from './environments/environment'; 
        
                   if (environment.production) { 
        
                     enableProdMode(); 
        
                   } 
        
                   platformBrowserDynamic().bootstrapModule(AppModule); 
        
                   import * as m from './app/app.module'; 
        
                   console.log(m.X); 
        
                   console.log('$$_E2E_GOLDEN_VALUE_3'); 
        
                 `, 
        
               }; 
        
               const overrides = { watch: true }; 
        
               let buildNumber = 0; 
        
               runTargetSpec(host, browserTargetSpec, overrides).pipe( 
        
                 // We must debounce on watch mode because file watchers are not very accurate. 
        
                 // Changes from just before a process runs can be picked up and cause rebuilds. 
        
                 // In this case, cleanup from the test right before this one causes a few rebuilds. 
        
                 debounceTime(1000), 
        
                 tap((buildEvent) => expect(buildEvent.success).toBe(true)), 
        
                 tap(() => { 
        
                   buildNumber += 1; 
        
                   switch (buildNumber) { 
        
                     case 1: 
        
                       // No lazy chunk should exist. 
        
                       expect(host.scopedSync().exists(join(outputPath, 'lazy-module.js'))).toBe(false); 
        
                       // Write the lazy chunk files. Order matters when writing these, because of imports. 
        
                       host.writeMultipleFiles(lazyModuleFiles); 
        
                       host.writeMultipleFiles(lazyModuleImport); 
        
                       break; 
        
                     case 2: 
        
                       // A lazy chunk should have been with the filename. 
        
                       expect(host.scopedSync().exists(join(outputPath, 'lazy-lazy-module.js'))).toBe(true); 
        
                       host.writeMultipleFiles(goldenValueFiles); 
        
                       break; 
        
                     case 3: 
        
                       // The golden values should be present and in the right order. 
        
                       const re = new RegExp( 
        
                         /\$\$_E2E_GOLDEN_VALUE_1(.|\n|\r)*/.source 
        
                         + /\$\$_E2E_GOLDEN_VALUE_2(.|\n|\r)*/.source 
        
                         + /\$\$_E2E_GOLDEN_VALUE_3/.source, 
        
                       ); 
        
                       const fileName = './dist/main.js'; 
        
                       const content = virtualFs.fileBufferToString( 
        
                         host.scopedSync().read(normalize(fileName)), 
        
                       ); 
        
                       expect(content).toMatch(re); 
        
                       break; 
        
                     default: 
        
                       break; 
        
                   } 
        
                 }), 
        
                 take(3), 
        
               ).toPromise().then(done, done.fail); 
        
             }, Timeout.Massive);

The appveyor error (below) you are seeing is unrelated to your PR though. It's something I have been unable to catch properly on appveyor builds. Will look at in again today.

  19 Browser Builder rebuilds
    √ rebuilds on TS file changes (19 secs)
Error: ENOTEMPTY: directory not empty, rmdir 'C:\projects\angular-cli\tests\@angular_devkit\build_angular\test-project-host-hello-world-app-5t0wtykex0e\src'
    at Object.fs.rmdirSync (fs.js:821:3)
    at MergeMapSubscriber.isDirectory.pipe.operators_1.concatMap.isDir [as project] (C:\projects\angular-cli\packages\angular_devkit\core\node\file:\C:\projects\angular-cli\packages\angular_devkit\core\node\host.ts:273:16)
    at MergeMapSubscriber._tryNext (C:\pr

BTW regarding timing, this PR cannot go in for 6.1 because we are already in RC. But it can go in for 6.2.

alan-agius4 · 2018-07-02T11:41:38Z

Thanks @filipesilva, that was really helpful.

I have added the unit tests.

Ps: I love how the Virtual FileSystem thingy works in the CLI. Big kudos guys :)

alan-agius4 · 2018-07-02T13:31:48Z

package-lock.json

@@ -180,7 +180,7 @@
    "@types/estree": {


Not sure why so many changes, I am using npm --version 6.1.0

Looks like it was due to a private npm repo

alan-agius4 · 2018-07-02T15:30:40Z

@filipesilva I am now having the rmdir error on my newly added tests aswell. :/

filipesilva · 2018-07-03T11:54:45Z

Yeah we are trying to do a FS abstraction with the Hosts but still have a couple of parts missing for true cross platform support. Glad you like it!

I think the current rmdir error you have is legit. To make watch mode work with tooling, it is necessary that the watcher can exit gracefully.

For webpack, that happens in

angular-cli/packages/angular_devkit/build_webpack/src/webpack/index.ts

Lines 71 to 72 in 47f937e

    
           // Teardown logic. Close the watcher when unsubscribed from. 
        
           return () => watching.close(() => { });

This is actually not as big a problem in linux/osx as it is on windows.

In linux/osx, if you don't exit the watchers gracefully, you get hanging process. In our test runner that isn't a problem because we do process.exit at the end. We do this because we have found some situations when processes still hang in webpack and in karma.

In windows however, you get errors when removing files because the watcher is keeping a file lock.

So I think you need a API to tell ng-packagr to exit gracefully in order to use it with tooling (such as the ng-packagr builder).

alan-agius4 · 2018-07-03T12:14:07Z

Ah year, the teardown logic makes perfect sense as I am doing the subscription internally and I was never unsubscribing.

It was odd though because I didn't have the same errors when running the tests locally on a Windows machine.

Internally ng-packagr, will close the watch on unsubscribe, as it has it's own teardown logic https://github.com/dherges/ng-packagr/blob/master/src/lib/file/file-watcher.ts#L36

Ps: teardown logic added.

filipesilva

LGTM

Excited to get this in for the 6.2 beta, I feel a lot of users are going to benefit from the work you've done in ng-packagr watch mode!

filipesilva · 2018-07-05T13:19:10Z

packages/angular_devkit/build_ng_packagr/src/build/index.ts

+  try {
+    ngPackagrJsonPath = resolveProjectModule(projectRoot, 'ng-packagr/package.json');
+  } catch {
+    // @angular/service-worker is not installed


Nit: comment is lying.

Not really important to get this merged but should be changed if a rebase is needed.

I will rebase and do it :)

filipesilva · 2018-07-05T13:23:33Z

Eventually https://github.com/angular/angular-cli/blob/master/docs/documentation/stories/create-library.md should be updated with the watch mode instead of directing users to rebuild manually.

alan-agius4 · 2018-07-05T14:16:00Z

@filipesilva docs have been updated, would you mind review it again please?

Thanks

Ps: I am really excited about this feature as well. From the little feedback I go from users using ng-packagr directly it improved their experience quite a bit. 😄

filipesilva · 2018-07-06T01:55:27Z

Doc changes look good too, great work!

We'll be looking at getting this merged after 6.1 final, which is when we will release 6.2.0-beta.0.

pterblgh · 2018-07-06T08:53:15Z

Correct me if I'm wrong, but this feature will make available for monorepos with multiple libraries to live-reload the main application which uses the libraries?

Currently I have a working setup with multiple libraries and a consuming app with lerna, but the developer experience is not quite good because every time someone makes a change in a library it's needed to be rebuilt. Is this feature going to solve this problem?

alan-agius4 · 2018-07-06T09:03:55Z

Hi, while I don’t have full picture of your architecture with lerna. However this should work, by spawning 2 process one that watches for the changes in the library and the other one that is watching the app. So both library and app will do a partial re-build.

pterblgh · 2018-07-06T09:10:37Z

Thanks for the quick answer @alan-agius4, I'm using Angular CLI for both building the libraries and the main application. I guess I will setup a lerna command to execute ng build --watch in the libraries and to start the main application.

If everything goes well I guess it should work correctly, because right now if I have a running main app in a process and I trigger a manual library build, the main app partially re-builds as expected.

smoke · 2018-07-06T20:05:37Z

@pterblgh You don't need lerna, I have setup the following scripts https://github.com/ngx-api-utils/ngx-api-utils/blob/master/package.json#L13 and it works quite well. For library / package rebuild on change I am using https://github.com/ngx-api-utils/ngx-api-utils/blob/master/package.json#L24 so you can also have nodemon doing it.
Of course when we have watch when building a library, speed will be increased because of the incremental builds.
So I am quite eager seeing this PR in @angular/cli!

mina-skunk · 2018-07-19T19:36:51Z

packages/angular_devkit/build_ng_packagr/src/build/index.ts

-          obs.complete();
-        })
-        .catch((e) => obs.error(e));
+      if (options.watch) {


Since ng-packagr build() uses buildAsObservable() internally why not remove the if and the new Observable statement and do return ngPkgProject..withOptions({ watch: options.watch })buildAsObservable().pipe(map(() => {return { success: true };}))

The reason being is that angular cli still needs to support ng-package v2 and v3. And this method is only available in ng-packagr v4

Never mind, I see, for backwards compatibility with ng-packagr 3.x when not using watch, right?

alan-agius4 · 2018-07-22T09:04:04Z

Will eventually rebase when it's ready to get merged. Also, I created another PR to bump ng-packagr to v4 final

#11622

filipesilva · 2018-07-25T21:58:16Z

@alan-agius4 should be ok to rebase now, #11622 is in as well.

…brary `ng-packagr` version `4.0.0-rc.3`, lands the incremental builds feature. More info: https://github.com/dherges/ng-packagr/blob/master/CHANGELOG.md#400-rc2-2018-06-23 `enableResourceInlining` needs to be enabled for libraries that contain components Closes: #11100

…building a library `ng-packagr` version `4.0.0-rc.3`, lands the incremental builds feature. More info: https://github.com/dherges/ng-packagr/blob/master/CHANGELOG.md#400-rc2-2018-06-23 `enableResourceInlining` needs to be enabled for libraries that contain components Closes: #10643

alan-agius4 · 2018-07-26T05:17:45Z

thanks @filipesilva, I just rebased.

angular-automatic-lock-bot · 2019-09-12T12:41:31Z

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 Jun 24, 2018

filipesilva suggested changes Jul 2, 2018

View reviewed changes

filipesilva added the target: minor label Jul 2, 2018

alan-agius4 commented Jul 2, 2018

View reviewed changes

filipesilva previously approved these changes Jul 5, 2018

View reviewed changes

alan-agius4 dismissed filipesilva’s stale review via 3631ebf July 5, 2018 14:00

filipesilva previously approved these changes Jul 6, 2018

View reviewed changes

mina-skunk reviewed Jul 19, 2018

View reviewed changes

alan-agius4 mentioned this pull request Jul 22, 2018

update ng-packagr to version 4 #11622

Merged

alan-agius4 dismissed filipesilva’s stale review via 10683b3 July 26, 2018 05:14

alan-agius4 added 4 commits July 26, 2018 07:16

test(@angular-devkit/build-ng-packagr): add tests for watch mode

710f075

docs: update create library docs and include watch

1e5d74a

filipesilva approved these changes Jul 26, 2018

View reviewed changes

hansl merged commit 9825f73 into angular:master Jul 30, 2018

alan-agius4 deleted the ng-packagr_watch branch July 30, 2018 13:42

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

	if (!semver.gte(swVersion, NEW_SW_VERSION)) {
	throw new Error(tags.stripIndent`
	The installed version of @angular/service-worker is ${swVersion}. This version of the CLI
	requires the @angular/service-worker version to satisfy ${NEW_SW_VERSION}. Please upgrade
	your service worker version.
	`);
	}

	Version.assertCompatibleAngularVersion(this.project.root);
	Version.assertTypescriptVersion(this.project.root);

feat: add support for watch when building a library #11358

feat: add support for watch when building a library #11358

Uh oh!

Conversation

alan-agius4 commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Jul 2, 2018 • 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!

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 Jul 2, 2018 • 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 commented Jul 2, 2018

Uh oh!

alan-agius4 commented Jul 2, 2018

Uh oh!

filipesilva commented Jul 2, 2018

Uh oh!

alan-agius4 commented Jul 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-agius4 commented Jul 2, 2018

Uh oh!

filipesilva commented Jul 3, 2018

Uh oh!

alan-agius4 commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipesilva 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!

filipesilva commented Jul 5, 2018

Uh oh!

alan-agius4 commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipesilva commented Jul 6, 2018

Uh oh!

pterblgh commented Jul 6, 2018

Uh oh!

alan-agius4 commented Jul 6, 2018

Uh oh!

pterblgh commented Jul 6, 2018

Uh oh!

feat: add support for `watch` when building a library #11358

feat: add support for `watch` when building a library #11358

alan-agius4 commented Jun 24, 2018 •

edited

Loading

alan-agius4 Jul 2, 2018 •

edited

Loading

alan-agius4 Jul 2, 2018 •

edited

Loading

alan-agius4 commented Jul 3, 2018 •

edited

Loading

alan-agius4 commented Jul 5, 2018 •

edited

Loading

smoke commented Jul 6, 2018 •

edited

Loading