Skip to content

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

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

Merged
merged 4 commits into from
Jul 30, 2018
Merged
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
33 changes: 14 additions & 19 deletions docs/documentation/stories/create-library.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,29 +78,24 @@ directory for the library beforehand, removing old code leftover code from previ
## Why do I need to build the library everytime I make changes?

Running `ng build my-lib` every time you change a file is bothersome and takes time.
In `Angular CLI` version `6.2` an incremental builds functionality has been added to improve the experience of library developers.
Everytime a file is changed a partial build is performed that emits the amended files.

Some similar setups instead add the path to the source code directly inside tsconfig.
This makes seeing changes in your app faster.
The feature can be using by passing `--watch` command argument as show below;

But doing that is risky.
When you do that, the build system for your app is building the library as well.
But your library is built using a different build system than your app.

Those two build systems can build things slightly different, or support completely different
features.

This leads to subtle bugs where your published library behaves differently from the one in your
development setup.

For this reason we decided to err on the side of caution, and make the recommended usage
the safe one.

In the future we want to add watch support to building libraries so it is faster to see changes.
```bash
ng build my-lib --watch
```

We are also planning to add internal dependency support to Angular CLI.
This means that Angular CLI would know your app depends on your library, and automatically rebuilds
the library when the app needs it.
Note: This feature requires that Angular's Compiler Option [enableResourceInlining](https://angular.io/guide/aot-compiler#enableresourceinlining) is enabled.
This can be done by adding the below in your `tsconfig.lib.json`.

```javascript
"angularCompilerOptions": {
"enableResourceInlining": true,
...
}
```

## Note for upgraded projects

Expand Down
53 changes: 41 additions & 12 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/angular_devkit/build_ng_packagr/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"dependencies": {
"@angular-devkit/architect": "0.0.0",
"@angular-devkit/core": "0.0.0",
"rxjs": "^6.0.0"
"rxjs": "^6.0.0",
"semver": "^5.3.0"
},
"peerDependencies": {
"ng-packagr": "^2.2.0 || ^3.0.0 || ^4.0.0"
Expand Down
69 changes: 61 additions & 8 deletions packages/angular_devkit/build_ng_packagr/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,55 @@ import {
BuilderConfiguration,
BuilderContext,
} from '@angular-devkit/architect';
import { getSystemPath, normalize, resolve } from '@angular-devkit/core';
import { getSystemPath, normalize, resolve, tags } from '@angular-devkit/core';
import * as fs from 'fs';
import * as ngPackagr from 'ng-packagr';
import { Observable } from 'rxjs';
import { EMPTY, Observable } from 'rxjs';
import { catchError, tap } from 'rxjs/operators';
import * as semver from 'semver';

const NEW_NG_PACKAGR_VERSION = '4.0.0-rc.3';

// TODO move this function to architect or somewhere else where it can be imported from.
// Blatantly copy-pasted from 'require-project-module.ts'.
function requireProjectModule(root: string, moduleName: string) {
return require(require.resolve(moduleName, { paths: [root] }));
}

function resolveProjectModule(root: string, moduleName: string) {
return require.resolve(moduleName, { paths: [root] });
}

export interface NgPackagrBuilderOptions {
project: string;
tsConfig?: string;
watch?: boolean;
}

function checkNgPackagrVersion(projectRoot: string): boolean {
let ngPackagrJsonPath;

try {
ngPackagrJsonPath = resolveProjectModule(projectRoot, 'ng-packagr/package.json');
} catch {
// ng-packagr is not installed
throw new Error(tags.stripIndent`
ng-packagr is not installed. Run \`npm install ng-packagr --save-dev\` and try again.
`);
}

const ngPackagrPackageJson = fs.readFileSync(ngPackagrJsonPath).toString();
const ngPackagrVersion = JSON.parse(ngPackagrPackageJson)['version'];

if (!semver.gte(ngPackagrVersion, NEW_NG_PACKAGR_VERSION)) {
throw new Error(tags.stripIndent`
The installed version of ng-packagr is ${ngPackagrVersion}. The watch feature
requires ng-packagr version to satisfy ${NEW_NG_PACKAGR_VERSION}.
Please upgrade your ng-packagr version.
`);
}

return true;
}

export class NgPackagrBuilder implements Builder<NgPackagrBuilderOptions> {
Expand Down Expand Up @@ -53,12 +88,30 @@ export class NgPackagrBuilder implements Builder<NgPackagrBuilderOptions> {
ngPkgProject.withTsConfig(tsConfigPath);
}

ngPkgProject.build()
.then(() => {
obs.next({ success: true });
obs.complete();
})
.catch((e) => obs.error(e));
if (options.watch) {

Choose a reason for hiding this comment

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

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 };}))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

checkNgPackagrVersion(getSystemPath(root));

const ngPkgSubscription = ngPkgProject
.watch()
.pipe(
tap(() => obs.next({ success: true })),
catchError(e => {
obs.error(e);

return EMPTY;
}),
)
.subscribe();

return () => ngPkgSubscription.unsubscribe();
} else {
ngPkgProject.build()
.then(() => {
obs.next({ success: true });
obs.complete();
})
.catch(e => obs.error(e));
}
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@

import { TargetSpecifier } from '@angular-devkit/architect';
import { TestProjectHost, runTargetSpec } from '@angular-devkit/architect/testing';
import { join, normalize } from '@angular-devkit/core';
import { tap } from 'rxjs/operators';

import { join, normalize, virtualFs } from '@angular-devkit/core';
import { debounceTime, map, take, tap } from 'rxjs/operators';

const devkitRoot = normalize((global as any)._DevKitRoot); // tslint:disable-line:no-any
const workspaceRoot = join(devkitRoot, 'tests/angular_devkit/build_ng_packagr/ng-packaged/');
Expand Down Expand Up @@ -43,4 +42,59 @@ describe('NgPackagr Builder', () => {
tap((buildEvent) => expect(buildEvent.success).toBe(true)),
).toPromise().then(done, done.fail);
});

it('rebuilds on TS file changes', (done) => {
const targetSpec: TargetSpecifier = { project: 'lib', target: 'build' };

const goldenValueFiles: { [path: string]: string } = {
'projects/lib/src/lib/lib.component.ts': `
import { Component } from '@angular/core';

@Component({
selector: 'lib',
template: 'lib update works!'
})
export class LibComponent { }
`,
};

const overrides = { watch: true };

let buildNumber = 0;

runTargetSpec(host, targetSpec, 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)),
map(() => {
const fileName = './dist/lib/fesm5/lib.js';
const content = virtualFs.fileBufferToString(
host.scopedSync().read(normalize(fileName)),
);

return content;
}),
tap(content => {
buildNumber += 1;
switch (buildNumber) {
case 1:
expect(content).toMatch(/lib works/);
host.writeMultipleFiles(goldenValueFiles);
break;

case 2:
expect(content).toMatch(/lib update works/);
break;
default:
break;
}
}),
take(2),
)
.toPromise()
.then(done, done.fail);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
"tsConfig": {
"type": "string",
"description": "The file path of the TypeScript configuration file."
},
"watch": {
"type": "boolean",
"description": "Run build when files change.",
"default": false
}
},
"additionalProperties": false,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{
"$schema": "<%= relativePathToWorkspaceRoot %>/node_modules/ng-packagr/ng-package.schema.json",
"dest": "<%= relativePathToWorkspaceRoot %>/<%= distRoot %>",
"deleteDestPath": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Jul 2, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

"lib": {
"entryFile": "src/<%= entryFile %>.ts"
}
Expand Down

This file was deleted.

6 changes: 0 additions & 6 deletions packages/schematics/angular/library/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import { latestVersions } from '../utility/latest-versions';
import { validateProjectName } from '../utility/validation';
import { Schema as LibraryOptions } from './schema';


interface UpdateJsonFn<T> {
(obj: T): T | void;
}
Expand Down Expand Up @@ -145,11 +144,6 @@ function addAppToWorkspaceFile(options: LibraryOptions, workspace: WorkspaceSche
tsConfig: `${projectRoot}/tsconfig.lib.json`,
project: `${projectRoot}/ng-package.json`,
},
configurations: {
production: {
project: `${projectRoot}/ng-package.prod.json`,
},
},
},
test: {
builder: '@angular-devkit/build-angular:karma',
Expand Down
1 change: 0 additions & 1 deletion packages/schematics/angular/library/index_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ describe('Library Schematic', () => {
const fileContent = getJsonFileContent(tree, '/projects/foo/ng-package.json');
expect(fileContent.lib).toBeDefined();
expect(fileContent.lib.entryFile).toEqual('src/my_index.ts');
expect(fileContent.deleteDestPath).toEqual(false);
expect(fileContent.dest).toEqual('../../dist/foo');
});

Expand Down