feat(ivy): support ng-add in localize package #32791

filipesilva · 2019-09-20T15:08:05Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the new behavior?

The @angular/localize package can by added to an Angular CLI project via ng add.

Does this PR introduce a breaking change?

Yes
No

Other information

mary-poppins · 2019-09-20T15:18:24Z

You can preview 90debce at https://pr32791-90debce.ngbuilds.io/.

mary-poppins · 2019-09-20T15:30:32Z

You can preview 39abc30 at https://pr32791-39abc30.ngbuilds.io/.

mary-poppins · 2019-09-20T16:21:12Z

You can preview 8753f5d at https://pr32791-8753f5d.ngbuilds.io/.

mary-poppins · 2019-09-20T17:50:23Z

You can preview f4ac0cc at https://pr32791-f4ac0cc.ngbuilds.io/.

petebacondarwin

Thanks for getting on with this @filipesilva .
The schematic itself looks good to me.
I am concerned about the extra dependencies and I think we need a README file to explain what this schematic is doing and why.

packages/localize/package.json

packages/localize/schematics/ng-add/index.ts

packages/localize/schematics/ng-add/schema.json

packages/localize/schematics/ng-add/index_spec.ts

petebacondarwin · 2019-09-23T06:22:42Z

packages/localize/schematics/ng-add/index.ts

+
+
+// Object.values() is the right way of doing this but it is only available with the ES2017
+// TS lib and is not downleveled.


What versions of node do we have to support?
Object.values has been available since node.js v7.

Only Node 10 and above on the CLI side of things, which is the main consumer for ng-add. Is it ok to add compilation units with custom tsconfigs here though? None of the other packages seem to do it.

I think it makes sense to target the node version of the consuming code (CLI in this case). You could view the schematic as a specialized secondary entrypoint for the package with its own set of features and requirements.

NIT: I think the consensus is that this should be removed? But not a blocker on merging this PR.

Removing it means using a different tsconfig for building this target that includes the es2017.object lib. I haven't gotten an answer to if that's ok, but since no other package seems to do it I think it would need approval from someone like @alexeagle.

Spoke to Pete, he showed me where in compiler-cli he did the exact same thing, it's now using the extra lib and using commonjs modules.

petebacondarwin · 2019-09-23T06:31:24Z

Oh and up to now all localize related commits have used the ivy scope.
Have you discussed adding this localize scope with anyone?

filipesilva · 2019-09-23T13:29:13Z

@petebacondarwin I didn't know that the localize package should use ivy scopes. I haven't discussed it with anyone, just assumed the scope should be the package name. Will remove the change.

mary-poppins · 2019-09-23T13:46:28Z

You can preview 9364875 at https://pr32791-9364875.ngbuilds.io/.

packages/localize/schematics/ng-add/index.ts

alan-agius4

See above.

filipesilva · 2019-09-25T10:46:24Z

@petebacondarwin added the README you requested as well.

mary-poppins · 2019-09-25T10:53:35Z

You can preview ed51d57 at https://pr32791-ed51d57.ngbuilds.io/.

packages/localize/schematics/ng-add/index.ts

packages/localize/schematics/ng-add/index_spec.ts

mary-poppins · 2019-09-25T11:25:36Z

You can preview 5680042 at https://pr32791-5680042.ngbuilds.io/.

mary-poppins · 2019-09-25T11:40:15Z

You can preview 5be4ade at https://pr32791-5be4ade.ngbuilds.io/.

mary-poppins · 2019-09-25T11:58:16Z

You can preview 61cd8a2 at https://pr32791-61cd8a2.ngbuilds.io/.

packages/localize/schematics/ng-add/index.ts

mary-poppins · 2019-09-25T15:34:33Z

You can preview 4656c81 at https://pr32791-4656c81.ngbuilds.io/.

alan-agius4

LGTM

gkalpak

LGTM for dev-infra.

gkalpak · 2019-09-26T10:49:29Z

packages/localize/schematics/ng-add/README.md

+
+This schematic will be executed when a Angular CLI user runs `ng add @angular/localize`.
+
+It will search their `.angular.json` file, and find polyfills and main files for server builders.


.angular.json --> angular.json

gkalpak · 2019-09-26T11:04:02Z

packages/localize/schematics/ng-add/schema.json

+{
+  "$schema": "http://json-schema.org/schema",
+  "id": "SchematicsAngularBazelNgAdd",
+  "title": "Angular Bazel Ng Add Schema",


Why "Bazel"? 😕

mary-poppins · 2019-09-26T11:49:40Z

You can preview a705927 at https://pr32791-a705927.ngbuilds.io/.

atscott · 2019-09-27T17:11:43Z

Presubmit
Ivy Presubmit

The scheamtics added in angular#32791 is currently failing as the package.json does not reference it. ``` > ng add @angular/localize@9.0.0-next.9 + @angular/localize@9.0.0-next.9 added 1 package from 1 contributor in 6.745s Installed packages for tooling via npm. The package that you are trying to add does not support schematics. You can try using a different version of the package or contact the package author to add ng-add support. ```

The schematics added in angular#32791 is currently failing as the package.json does not reference it. ``` > ng add @angular/localize@9.0.0-next.9 + @angular/localize@9.0.0-next.9 added 1 package from 1 contributor in 6.745s Installed packages for tooling via npm. The package that you are trying to add does not support schematics. You can try using a different version of the package or contact the package author to add ng-add support. ```

The schematics added in #32791 is currently failing as the package.json does not reference it. ``` > ng add @angular/localize@9.0.0-next.9 + @angular/localize@9.0.0-next.9 added 1 package from 1 contributor in 6.745s Installed packages for tooling via npm. The package that you are trying to add does not support schematics. You can try using a different version of the package or contact the package author to add ng-add support. ``` PR Close #33025

The schematics added in angular#32791 is currently failing as the package.json does not reference it. ``` > ng add @angular/localize@9.0.0-next.9 + @angular/localize@9.0.0-next.9 added 1 package from 1 contributor in 6.745s Installed packages for tooling via npm. The package that you are trying to add does not support schematics. You can try using a different version of the package or contact the package author to add ng-add support. ``` PR Close angular#33025

angular-automatic-lock-bot · 2019-10-28T16:07:41Z

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 Sep 20, 2019

filipesilva requested review from petebacondarwin and clydin September 20, 2019 15:08

filipesilva added the target: major This PR is targeted for the next major release label Sep 20, 2019

filipesilva marked this pull request as ready for review September 20, 2019 18:16

filipesilva requested review from a team as code owners September 20, 2019 18:16

petebacondarwin requested changes Sep 23, 2019

View reviewed changes

filipesilva changed the title ~~Ng add localize~~ feat(ivy): support ng-add in localize Sep 23, 2019

filipesilva changed the title ~~feat(ivy): support ng-add in localize~~ feat(ivy): support ng-add in localize package Sep 23, 2019

alan-agius4 reviewed Sep 24, 2019

View reviewed changes

packages/localize/schematics/ng-add/index.ts Outdated Show resolved Hide resolved

alan-agius4 requested changes Sep 24, 2019

View reviewed changes

filipesilva requested review from petebacondarwin, alan-agius4 and gkalpak September 25, 2019 10:46

petebacondarwin reviewed Sep 25, 2019

View reviewed changes

packages/localize/schematics/ng-add/index.ts Show resolved Hide resolved

petebacondarwin reviewed Sep 25, 2019

View reviewed changes

packages/localize/schematics/ng-add/index_spec.ts Outdated Show resolved Hide resolved

petebacondarwin approved these changes Sep 25, 2019

View reviewed changes

alan-agius4 reviewed Sep 25, 2019

View reviewed changes

packages/localize/schematics/ng-add/index.ts Outdated Show resolved Hide resolved

alan-agius4 reviewed Sep 25, 2019

View reviewed changes

packages/localize/schematics/ng-add/index.ts Outdated Show resolved Hide resolved

alan-agius4 approved these changes Sep 25, 2019

View reviewed changes

AndrewKushnir added the comp: ivy label Sep 25, 2019

ngbot bot added this to the needsTriage milestone Sep 25, 2019

filipesilva removed the request for review from clydin September 26, 2019 10:06

gkalpak approved these changes Sep 26, 2019

View reviewed changes

gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 26, 2019

feat(ivy): support ng-add in localize package

a705927

filipesilva removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 26, 2019

filipesilva added the action: merge The PR is ready for merge by the caretaker label Sep 27, 2019

vikerman approved these changes Sep 27, 2019

View reviewed changes

atscott added the action: presubmit The PR is in need of a google3 presubmit label Sep 27, 2019

atscott closed this in e41cbfb Sep 27, 2019

cexbrayat mentioned this pull request Oct 7, 2019

fix(ivy): missing schematics field in localize package #33025

Closed

14 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ivy): support ng-add in localize package #32791

feat(ivy): support ng-add in localize package #32791

filipesilva commented Sep 20, 2019

mary-poppins commented Sep 20, 2019

mary-poppins commented Sep 20, 2019

mary-poppins commented Sep 20, 2019

mary-poppins commented Sep 20, 2019

petebacondarwin left a comment

petebacondarwin Sep 23, 2019

filipesilva Sep 23, 2019

clydin Sep 23, 2019

petebacondarwin Sep 25, 2019

filipesilva Sep 25, 2019

filipesilva Sep 25, 2019

petebacondarwin commented Sep 23, 2019

filipesilva commented Sep 23, 2019

mary-poppins commented Sep 23, 2019

alan-agius4 left a comment

filipesilva commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

alan-agius4 left a comment

gkalpak left a comment

gkalpak Sep 26, 2019

gkalpak Sep 26, 2019

petebacondarwin Sep 26, 2019

filipesilva Sep 26, 2019

mary-poppins commented Sep 26, 2019

atscott commented Sep 27, 2019

angular-automatic-lock-bot bot commented Oct 28, 2019



		// Object.values() is the right way of doing this but it is only available with the ES2017
		// TS lib and is not downleveled.


		This schematic will be executed when a Angular CLI user runs `ng add @angular/localize`.

		It will search their `.angular.json` file, and find polyfills and main files for server builders.

feat(ivy): support ng-add in localize package #32791

feat(ivy): support ng-add in localize package #32791

Conversation

filipesilva commented Sep 20, 2019

PR Checklist

PR Type

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mary-poppins commented Sep 20, 2019

mary-poppins commented Sep 20, 2019

mary-poppins commented Sep 20, 2019

mary-poppins commented Sep 20, 2019

petebacondarwin left a 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

petebacondarwin commented Sep 23, 2019

filipesilva commented Sep 23, 2019

mary-poppins commented Sep 23, 2019

alan-agius4 left a comment

Choose a reason for hiding this comment

filipesilva commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

mary-poppins commented Sep 25, 2019

alan-agius4 left a comment

Choose a reason for hiding this comment

gkalpak left a 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

mary-poppins commented Sep 26, 2019

atscott commented Sep 27, 2019

angular-automatic-lock-bot bot commented Oct 28, 2019