feat: build architect for ng-packagr #444

dherges · 2018-02-18T18:05:41Z

Schematic for library scaffolding:

ng new library foo will
- create folder lib/foo/package.json with ngPackage config
- update tsconfig for dev experience
- update root package.json with additional dependencies

Build architect for ng-packagr:

forward project param from ng-packagr

googlebot · 2018-02-18T18:05:46Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

dherges · 2018-02-18T18:15:03Z

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

+        .forProject(packageJsonPath)
+        .build()
+        .then(() => obs.complete())
+        .catch((e: any) => obs.error(e));


At the moment, the only way is passing a file path in either ngPackagr.forProject() or in the build() command

dherges · 2018-03-07T08:47:55Z

packages/schematics/angular/library/index.ts

+      if (!json.scripts) {
+        json.scripts = {};
+      }
+      json.scripts[`libs:${name}:build`] = `ng-packagr -p ${sourceDir}/package.json`;


Should the script invoke ng-packagr directly or run through the build architect?

I think the ng command to build is the one that should be called here, which in turn would call architect with the workspace setup.

Although I do wonder if we should be adding the individual scripts at all when adding a new project/lib via schematic.

Can we just hook past your CLI? You should probably expose a JavaScript API that we can use directly.

Yes, should the schematic create the workspace setup for Builder/Runner/Architect? What is the command users will execute then?

dherges · 2018-03-07T20:09:12Z

windows test failures due to path delimeters. https://ci.appveyor.com/project/AngularCLI/devkit/build/1.0.740#L82
loose json parsing, tolerates comments .. I think I cannot use this as I need to serialize back to JSON

…ckagr

filipesilva · 2018-03-11T16:27:34Z

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

+    }
+
+    return new Observable(obs => {
+      const projectNgPackagr = requireProjectModule(root, 'ng-packagr') as typeof ngPackagr;


I'm actually not sure either NgPackager, nor TsLint (where this setup code came from) should be loaded as project dependencies.

I think it should either be a peer dependency, or a direct dependency. Both of those approaches have pros and cons, but in either case I don't think we should try to short-circuit it by requiring the project module instead of importing directly.

In the case of NgPackagr, you already have it as a direct dependency so I think we can import it directly.

filipesilva · 2018-03-11T16:33:19Z

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

+    architect.loadWorkspaceFromJson(getWorkspace()).pipe(
+      concatMap((architect) => architect.run(architect.getTarget())),
+      toArray(),
+      tap(events => expect(events.length).toBe(0)),


At the when I linked you the TSlint example the tests there were a bit anemic. Since then I added more, and more involved ones.

I think here you should check the build event success, and also check if the output is what you expected. An example of a test that does that is https://github.com/angular/devkit/blob/master/packages/angular_devkit/build_webpack/test/tslint/works_spec_large.ts#L54-L67.

filipesilva · 2018-03-11T16:37:41Z

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

+      projectNgPackagr.ngPackagr()
+        .forProject(packageJsonPath)
+        .build()
+        .then(() => obs.complete())


Overall we're trying to go for a model where builders always emit events. This wasn't the case before when you started working on it but since then we revised that because otherwise it was harder to test and reason about it.

The general idea is:

whenever a builder finished a build, it will emit {success: true} if it succeeded or {success: false} if it did not

when a builder is done, it will complete. Builders on watch mode would emit multiple times and not complete, whereas single run builders should emit once and complete.

when there is an error, the builder should obs.error. Note that this is different from a failed build though, which emits a {success: false} and no error. I think the usage of error you have here is the right one.

filipesilva · 2018-03-11T16:39:41Z

tests/@angular_devkit/build_ng_packagr/ng-packaged/.angular-cli.json

@@ -0,0 +1,60 @@
+{


I know these files are needed for now but we need to review the final file set for adding a library to an existing project before wrapping up.

Should I strip the test to be a minimal sample? WOuld only be a package.json, a few *.ts/*.html/*.css files and then I'll verify the dist folder is created.

Yes that would be preferable, that way the tests only include what they need and it is easier to keep them updated.

hansl · 2018-03-14T19:56:44Z

Validating David's CLA, signed on Mar 31, 2017 02:43 PDT.

filipesilva · 2018-03-19T20:14:22Z

I followed up the comments in this PR in #532.

Followup to comments in #444

Followup to comments in angular/devkit#444

dherges requested a review from hansl as a code owner February 18, 2018 18:05

googlebot added the cla: no label Feb 18, 2018

dherges commented Feb 18, 2018

View reviewed changes

filipesilva self-assigned this Feb 26, 2018

filipesilva added the Work in Progress label Feb 26, 2018

juristr mentioned this pull request Mar 5, 2018

[RFC] Library Support and Boilerplate Comment Thread angular/angular-cli#6510

Closed

dherges requested a review from Brocco as a code owner March 5, 2018 21:46

dherges commented Mar 7, 2018

View reviewed changes

dherges added 4 commits March 10, 2018 07:19

build: add ng-packagr dep, update monorepo and readme

f9517f4

feat(@schematics/angular): add library scaffolding

435a315

feat(@angular-devkit/build-ng-packagr): add build architect for ng pa…

e9dc849

…ckagr

style: fix linting issues

27e268c

dherges changed the title ~~[WIP] build architect for ng-packagr~~ feat: build architect for ng-packagr Mar 10, 2018

fix(@schematics/angular): ensure forward slashes in library command

503aa2a

filipesilva reviewed Mar 11, 2018

View reviewed changes

hansl added cla: yes and removed cla: no labels Mar 14, 2018

nweldev mentioned this pull request Mar 16, 2018

Add ability to publish multiple libraries from a single nx workspace nrwl/nx#225

Closed

hansl merged commit 01f3af7 into angular:master Mar 19, 2018

filipesilva mentioned this pull request Mar 19, 2018

Update NgPackagr builder #532

Merged

hansl pushed a commit that referenced this pull request Mar 19, 2018

fix(@angular-devkit/build-ng-packagr): update builder

34003c0

Followup to comments in #444

hansl pushed a commit to angular/angular-cli that referenced this pull request Jun 6, 2018

fix(@angular-devkit/build-ng-packagr): update builder

21f728e

Followup to comments in angular/devkit#444

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: build architect for ng-packagr #444

feat: build architect for ng-packagr #444

dherges commented Feb 18, 2018 •

edited

Loading

googlebot commented Feb 18, 2018

dherges Feb 18, 2018

dherges Mar 7, 2018

filipesilva Mar 11, 2018

hansl Mar 11, 2018

dherges Mar 12, 2018 •

edited

Loading

dherges commented Mar 7, 2018 •

edited

Loading

filipesilva Mar 11, 2018

filipesilva Mar 11, 2018

filipesilva Mar 11, 2018

filipesilva Mar 11, 2018

dherges Mar 12, 2018

filipesilva Mar 19, 2018

hansl commented Mar 14, 2018

filipesilva commented Mar 19, 2018

feat: build architect for ng-packagr #444

feat: build architect for ng-packagr #444

Conversation

dherges commented Feb 18, 2018 • edited Loading

googlebot commented Feb 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

dherges Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

dherges commented Mar 7, 2018 • edited Loading

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

hansl commented Mar 14, 2018

filipesilva commented Mar 19, 2018

dherges commented Feb 18, 2018 •

edited

Loading

dherges Mar 12, 2018 •

edited

Loading

dherges commented Mar 7, 2018 •

edited

Loading