feat(upgrade): support multi-slot projection in upgrade/static #14282

petebacondarwin · 2017-02-03T14:05:24Z

Please check if the PR fulfills these 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)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

downgradedComponents in upgrade/static did not correctly project multi-slot content.

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

gkalpak · 2017-02-03T14:11:33Z

modules/@angular/upgrade/src/common/downgrade_component.ts

 *
 * The `inputs` and `outputs` are strings that map the names of properties to camelCased
 * attribute names. They are of the form `"prop: attr"`; or simply `"propAndAttr" where the
 * property and attribute have the same identifier.
 *
+ * The `selectors` are the strings that appear in the `ng-content` element's `select` attribute.


Maybe: element's select attribute --> elements' select attributes

gkalpak · 2017-02-03T14:16:22Z

modules/@angular/upgrade/test/common/downgrade_component_adaptor_spec.ts

@@ -0,0 +1,94 @@
+/**


Naming the file _adapter_ instead of _adaptor_ would be more consistent with the class name 😉

gkalpak · 2017-02-03T14:27:22Z

modules/@angular/upgrade/test/common/downgrade_component_adaptor_spec.ts

+        const selectorHelper = new NgContentSelectorHelper();
+        const fakeInjector = {get: function() { return selectorHelper; }};
+        return new DowngradeComponentAdapter(
+            'id', {component: null, selectors}, content, null, null, null, fakeInjector, null, null,


Too much mocking here (Travis complains) 😃
DowngradeComponentAdapter creates the componentScope in the constructor, so it needs a $scope with a $new method.

Or not enough mocking!

googlebot · 2017-02-03T16:56:00Z

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 they're okay 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.

googlebot · 2017-02-03T16:57:44Z

CLAs look good, thanks!

petebacondarwin · 2017-02-03T16:58:12Z

@gkalpak PTAL

petebacondarwin · 2017-02-03T16:58:40Z

I have checked that this code passes #14276

petebacondarwin · 2017-02-09T09:18:35Z

@IgorMinar or @mhevery can we get a LGTM on the public api file change?

gkalpak · 2017-02-09T10:31:58Z

modules/@angular/upgrade/src/common/downgrade_component_adapter.ts

@@ -6,13 +6,14 @@
 * found in the LICENSE file at https://angular.io/license
 */

+import {CssSelector, SelectorMatcher, createElementCssSelector} from '@angular/compiler';


This line will now be included in upgrade/static. Can someone (@alexeagle?) verify that this won't have unwanted side effects (such as pulling in too much code from @angular/compiler)?

Sorry this is obsured from you :(
We could maybe split the modules/@angular/upgrade tsconfig into two compilation units that match how we compile it internally.

The short answer is that we compile only these files

"modules/@angular/upgrade/src/common/**/*.ts", "modules/@angular/upgrade/src/static/**/*.ts", "modules/@angular/upgrade/src/facade/**/*.ts", "modules/@angular/upgrade/static.ts",

so this file would need to be moved out of common, or needs to be excluded from the compilation (and then not imported by anything else in static)

(because of limitations in the treeshaker, we don't include @angular/compiler as a transitive dependency of angular apps at all)

@alexeagle shall I move these functions into some common place, @angular/common(??) or maybe facade? Or should i just make a copy of the files (I feel very bad about that) into upgrade?

It seems like either 1) upgrade shouldn't use these because they are part of the compiler, or 2) they are not really part of the compiler and could move to common.

@tbosch or @IgorMinar ?

mhevery · 2017-02-09T18:18:53Z

Please resolve conflicts

petebacondarwin · 2017-02-09T21:37:30Z

Copy it is then. I will update the PR tomorrow.

petebacondarwin · 2017-02-10T11:08:17Z

Unfortunately, @mhevery, I don't think that making a copy is an option. While these three imports look small on the surface, they hide loads of code in the form of the HTML template parser.

mhevery · 2017-02-10T20:18:34Z

@petebacondarwin ok, lets put in in the facade then.

petebacondarwin · 2017-02-28T11:32:36Z

Actually looking at this again, I think we can accomplish what we want by copying the selector.ts file and two files from the ml_parser folder; and then copying over a couple of functions from the template_parser.

gkalpak · 2017-03-02T06:54:57Z

modules/@angular/upgrade/src/dynamic/upgrade_adapter.ts

@@ -561,7 +561,7 @@ export class UpgradeAdapter {
                    providers: [
                      {provide: $INJECTOR, useFactory: () => ng1Injector},
                      {provide: $COMPILE, useFactory: () => ng1Injector.get($COMPILE)},
-                      {provide: ContentProjectionHelper, useClass: DynamicContentProjectionHelper},
+                      {provide: NgContentSelectorHelper, useClass: DynamicNgContentSelectorHelper},


Couldn't we get rid of having to provide NgContentSelectorHelper by automaticalky populating info.selector in the dynamic version?

You mean do it here? https://github.com/petebacondarwin/angular/blob/8c88cca5f54b6eca51dcea3d3350946fe744e8a0/modules/%40angular/upgrade/src/dynamic/upgrade_adapter.ts#L194

Yes that sounds reasonable. I'll try it out.

Sadly we cannot do that because the downgradeComponent helper is called before we bootstrap, so the Compiler service is not available.
I guess we could rewrite everything so that this information could be computed lazily after bootstrap but I don't think the benefit of removing the NgContentSelectorHelper would warrant the effort and the churn.

chuckjaz · 2017-03-11T00:51:57Z

This PR needs to be rebased.

petebacondarwin · 2017-03-11T09:05:17Z

@chuckjaz - done.

chuckjaz · 2017-03-14T00:05:57Z

This PR looks like it needs to be rebased again.

Closes angular#14261

tbosch · 2017-03-14T02:37:36Z

packages/upgrade/src/common/downgrade_component_adapter.ts

+      const ngContentIndices: number[] = [];
+      const node = nodes[j];
+      const selector =
+          createElementCssSelector(node.nodeName.toLowerCase(), getAttributesAsArray(node));


Couldn't we use element.matches (see https://developer.mozilla.org/en-US/docs/Web/API/Element/matches)? Angular already verified during compiling the component that the selector is a "simple" selector that could be resolved via CssSelector, and CssSelector is a strict subset of the full css spec...

tbosch · 2017-03-14T02:38:33Z

packages/upgrade/src/common/ng_content_selector_helper.ts

+ *   ask the compiler for the selectors of a component.
+ */
+export class NgContentSelectorHelper {
+  getNgContentSelectors(info: ComponentInfo): string[] {


I think we should change ComponentFactory to have a getter ngContentSelectors. This way, we can use this information in AOT and JIT mode without the user specifying it...

tbosch · 2017-03-14T02:42:23Z

packages/upgrade/src/common/downgrade_component.ts

+ * that appear in the downgraded component's template.
+ * These selectors must be provided in the order that they appear in the template as they are
+ * mapped by index to the projected nodes.
+ *
 * @experimental
 */
 export function downgradeComponent(info: /* ComponentInfo */ {
  component: Type<any>;
  inputs?: string[];


Using a string array here will break with closure enhanced optimizations. Also, I think we should change the compiler to provide an inputs and outputs map in the ComponentFactory class, together with ngContentSelectors. Something like this:

class ComponentFactory<T> { // mapping from mangeled input name to non mangeled name. inputs: {[key: string]: string}; // mapping from mangeled output name to non mangeled name. outputs: {[key: string]: string}; // array of the selectors users in `ng-content`. ngContentSelectors: string[]; }

tbosch · 2017-03-14T02:45:11Z

Sorry, I missed this PR somehow... But I do think we should change this before cutting the next rc as

we probably want to deprecate the old way of specifying inputs and outputs as they break closure compiler.
we don't want users to repeat the selectors of ng-content (nor the inputs or outputs)
I would like to keep the CssSelector and parser classes private to the compiler, so that we can change it more easily...

petebacondarwin · 2017-03-14T06:56:37Z

@tbosch:
Removing boilerplate from the upgrade/static API would make my day :-)
What do we need to do to make the compiler changes happen?
When we deprecate the inputs/outputs then they would just have no effect, right? Since the JIT/AOT compilers will do the work going forward.

tbosch · 2017-03-14T22:44:44Z

See #15156 for the cleanups...

…ar#14282) Closes angular#14261

angular-automatic-lock-bot · 2019-09-11T01:21:51Z

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.}

petebacondarwin added comp: upgrade/static freq2: medium action: review The PR is still awaiting reviews from at least one requested reviewer type: bug/fix feature Issue that requests a new feature labels Feb 3, 2017

googlebot added the cla: yes label Feb 3, 2017

petebacondarwin mentioned this pull request Feb 3, 2017

[upgrade/static] Support targeted projection #14261

Closed

gkalpak requested changes Feb 3, 2017

View reviewed changes

petebacondarwin force-pushed the upgrade-static-projection branch from d929037 to 6e4d98d Compare February 3, 2017 16:55

googlebot added cla: no and removed cla: yes labels Feb 3, 2017

petebacondarwin force-pushed the upgrade-static-projection branch from 6e4d98d to 26e81d4 Compare February 3, 2017 16:57

googlebot added cla: yes and removed cla: no labels Feb 3, 2017

gkalpak approved these changes Feb 3, 2017

View reviewed changes

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 9, 2017

petebacondarwin requested review from mhevery and IgorMinar February 9, 2017 09:18

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker labels Feb 9, 2017

gkalpak reviewed Feb 9, 2017

View reviewed changes

mhevery approved these changes Feb 9, 2017

View reviewed changes

petebacondarwin force-pushed the upgrade-static-projection branch 2 times, most recently from d087015 to 8c88cca Compare February 28, 2017 14:02

gkalpak reviewed Mar 2, 2017

View reviewed changes

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 8, 2017

petebacondarwin force-pushed the upgrade-static-projection branch from 8c88cca to dbb8b53 Compare March 8, 2017 16:44

petebacondarwin force-pushed the upgrade-static-projection branch from dbb8b53 to 6d0e1f0 Compare March 11, 2017 09:04

chuckjaz added the pr_action: queued label Mar 13, 2017

feat(upgrade): support multi-slot projection in upgrade/static

88eb58b

Closes angular#14261

chuckjaz force-pushed the upgrade-static-projection branch from 6d0e1f0 to 88eb58b Compare March 14, 2017 00:16

chuckjaz merged commit 914797a into angular:master Mar 14, 2017

tbosch reviewed Mar 14, 2017

View reviewed changes

petebacondarwin deleted the upgrade-static-projection branch March 15, 2017 07:46

SamVerschueren pushed a commit to SamVerschueren/angular that referenced this pull request Mar 18, 2017

feat(upgrade): support multi-slot projection in upgrade/static (angul…

d7d543b

…ar#14282) Closes angular#14261

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

feat(upgrade): support multi-slot projection in upgrade/static (angul…

6ecf13c

…ar#14282) Closes angular#14261

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(upgrade): support multi-slot projection in upgrade/static (angul…

7484d55

…ar#14282) Closes angular#14261

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(upgrade): support multi-slot projection in upgrade/static #14282

feat(upgrade): support multi-slot projection in upgrade/static #14282

petebacondarwin commented Feb 3, 2017

gkalpak Feb 3, 2017

gkalpak Feb 3, 2017

gkalpak Feb 3, 2017

petebacondarwin Feb 3, 2017

googlebot commented Feb 3, 2017

googlebot commented Feb 3, 2017

petebacondarwin commented Feb 3, 2017

petebacondarwin commented Feb 3, 2017

petebacondarwin commented Feb 9, 2017

gkalpak Feb 9, 2017

alexeagle Feb 9, 2017

alexeagle Feb 9, 2017

petebacondarwin Feb 9, 2017

alexeagle Feb 9, 2017

mhevery commented Feb 9, 2017

petebacondarwin commented Feb 9, 2017

petebacondarwin commented Feb 10, 2017

mhevery commented Feb 10, 2017

petebacondarwin commented Feb 28, 2017 •

edited

Loading

gkalpak Mar 2, 2017

petebacondarwin Mar 2, 2017

petebacondarwin Mar 8, 2017

chuckjaz commented Mar 11, 2017

petebacondarwin commented Mar 11, 2017

chuckjaz commented Mar 14, 2017

tbosch Mar 14, 2017

tbosch Mar 14, 2017 •

edited

Loading

tbosch Mar 14, 2017 •

edited

Loading

tbosch commented Mar 14, 2017 •

edited

Loading

petebacondarwin commented Mar 14, 2017

tbosch commented Mar 14, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

feat(upgrade): support multi-slot projection in upgrade/static #14282

feat(upgrade): support multi-slot projection in upgrade/static #14282

Conversation

petebacondarwin commented Feb 3, 2017

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

googlebot commented Feb 3, 2017

googlebot commented Feb 3, 2017

petebacondarwin commented Feb 3, 2017

petebacondarwin commented Feb 3, 2017

petebacondarwin commented Feb 9, 2017

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

mhevery commented Feb 9, 2017

petebacondarwin commented Feb 9, 2017

petebacondarwin commented Feb 10, 2017

mhevery commented Feb 10, 2017

petebacondarwin commented Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckjaz commented Mar 11, 2017

petebacondarwin commented Mar 11, 2017

chuckjaz commented Mar 14, 2017

Choose a reason for hiding this comment

tbosch Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

tbosch Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

tbosch commented Mar 14, 2017 • edited Loading

petebacondarwin commented Mar 14, 2017

tbosch commented Mar 14, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

petebacondarwin commented Feb 28, 2017 •

edited

Loading

tbosch Mar 14, 2017 •

edited

Loading

tbosch Mar 14, 2017 •

edited

Loading

tbosch commented Mar 14, 2017 •

edited

Loading