feat(ivy): add support for the ngProjectAs attribute #22498

pkozlowski-opensource · 2018-02-28T16:47:13Z

This PR adds support for the ngProjectAs attribute.

The implementation in this PR requires us to store a CSS selector (from the <ng-content>'s select attribute) in both parsed and raw (un-parsed) form. The raw form is needed so we can easily compare it with a value of the ngProjectAs attribute.

@chuckjaz this PR changes the compiler's canonical spec for content projection, so the compiler's implementation will have to be updated accordingly.

vicb · 2018-02-28T18:09:20Z

packages/core/src/render3/instructions.ts

@@ -539,8 +539,10 @@ function setUpAttributes(native: RElement, attrs: string[]): void {

  const isProc = isProceduralRenderer(renderer);
  for (let i = 0; i < attrs.length; i += 2) {
-    isProc ? (renderer as ProceduralRenderer3).setAttribute(native, attrs[i], attrs[i | 1]) :
-             native.setAttribute(attrs[i], attrs[i | 1]);
+    if (attrs[i] !== NG_PROJECT_AS_ATTR_NAME) {


what about

const attrName = a[i]; const attrValue = a[i+1]; // ... use attrName & attrValue

vicb · 2018-02-28T18:10:36Z

packages/core/src/render3/instructions.ts

@@ -1262,7 +1266,7 @@ export function projectionDef(index: number, selectors?: CssSelector[]): void {
    // - there are selectors defined
    // - a node has a tag name / attributes that can be matched
    if (selectors && componentChild.tNode) {
-      const matchedIdx = matchingSelectorIndex(componentChild.tNode, selectors !);
+      const matchedIdx = matchingSelectorIndex(componentChild.tNode, selectors !, rawSelectors !);


You should be able to remove the ! from selectors ! (b/c if (selectors))

vicb · 2018-02-28T18:11:18Z

packages/core/src/render3/node_selector_matcher.ts

@@ -115,13 +115,30 @@ export function isNodeMatchingSelector(tNode: TNode, selector: CssSelector): boo
  return false;
 }

+function isNodeMatchingProjectAs(tNode: TNode, rawSelector: string): boolean {


This function went away with the recent code changes.

vicb · 2018-02-28T18:14:26Z

packages/core/src/render3/node_selector_matcher.ts

+  const nodeAttrs = tNode.attrs;
+  if (nodeAttrs != null) {
+    const ngProjectAsAttrIdx = nodeAttrs.indexOf(NG_PROJECT_AS_ATTR_NAME);
+    if (ngProjectAsAttrIdx > -1) {


if (ngProjectAsAttrIdx & 1 === 0) -> add a test

Should NG_PROJECT_AS_ATTR_NAME always come first and then we don't have to iterate ?

Should NG_PROJECT_AS_ATTR_NAME always come first and then we don't have to iterate ?

It would be a possible runtime optimisation at the expense of more work in the compiler. I will let @chuckjaz to decide.

if (ngProjectAsAttrIdx & 1 === 0) -> add a test

This function went away with the recent code changes.

vicb · 2018-02-28T18:19:00Z

packages/core/src/render3/node_selector_matcher.ts

  for (let i = 0; i < selectors.length; i++) {
-    if (isNodeMatchingSelector(tNode, selectors[i])) {
+    if (isNodeMatchingProjectAs(tNode, rawSelectors[i]) ||


Is this logic right ?
I think that when a node is ngProjectedAs we should not isNodeMatchingSelector

Oh, good catch! You are right, we should probably not match selectors if ngProjectAs is not matching. Changed the logic and added a test. Thnx!

mhevery · 2018-02-28T18:24:34Z

packages/core/src/render3/instructions.ts

@@ -1245,9 +1247,11 @@ export function directiveRefresh<T>(directiveIndex: number, elementIndex: number
 * each projected node belongs (it re-distributes nodes among "buckets" where each "bucket" is
 * backed by a selector).
 *
- * @param selectors
+ * @param selectors A collection of parsed CSS selectors
+ * @param rawSelectors A collection of CSS selectors in the raw, un-parsed form


I think rawSelectors needs more documentation. Why is it needed, What is the format., how to use it. Also why do we have two different formats? Can it be unified?

maybe textSelectors ?

Renamed to textSelectors

@mhevery documented here: https://github.com/angular/angular/pull/22498/files#diff-ce885db4223480bd4f7b78bd22b6f058R1284

chuckjaz

The changes to the compiler spec look good.

vicb

See inline comments

pkozlowski-opensource · 2018-03-01T11:01:00Z

@mhevery @vicb all the comments addressed, please have another look.

vicb · 2018-03-01T18:04:45Z

packages/core/src/render3/node_selector_matcher.ts

+  const nodeAttrs = tNode.attrs;
+  if (nodeAttrs != null) {
+    const ngProjectAsAttrIdx = nodeAttrs.indexOf(NG_PROJECT_AS_ATTR_NAME);
+    if (ngProjectAsAttrIdx > -1) {


this is still wrong see my previous comment.
Also please create a github issue to move the ngProjectAs attr first.

vicb · 2018-03-01T18:06:54Z

packages/core/src/render3/node_selector_matcher.ts

+    // if a node has the ngProjectAs attribute match it against unparsed selector
+    // match a node against a parsed selector only if ngProjectAs attribute is not present
+    if (ngProjectAsAttrVal === textSelectors[i] ||
+        ngProjectAsAttrVal === null && isNodeMatchingSelector(tNode, selectors[i])) {


nit: Wouldn't ngProjectAsAttrVal && ngProjectAsAttrVal === textSelectors[i] || isNodeMatchingSelector(tNode, selectors[i]) make more sense ? (with the same behavior)

that's actually wrong !
current impl is fine.

vicb · 2018-03-01T19:15:28Z

packages/core/test/render3/node_selector_matcher_spec.ts

@@ -183,4 +184,26 @@ describe('css selector matching', () => {
    });
  });

+  describe('getProjectAsAttrValue', function() {


nit describe('reading the ngProjectAs attribute value' => ...
b/c function names change.

vicb

1 nit on the test name

JoostK · 2018-03-01T20:36:05Z

Would it be possible to let the template compiler recognize the ngProjectAs attribute and have it perform the CSS compilation that is then stored on TNode, such that we may compare by compiled selector instead? That would save the need to have a duplicate representation at runtime and would also normalize for whitespace differences etc.

pkozlowski-opensource · 2018-03-01T20:42:51Z

@JoostK it would be certainly possible but would have drawbacks:

we would have to allocate a field for ngProjectAs on every TNode and this would be wasteful (memory-wise), especially that ngProjectAs is super-rare
comparing compiled version of CSS selectors would be much more expensive that just comparing strings.

I'm sure that there are ways of improving solution from this PR but I've considered what you are suggesting and decided against it for the reasons outlined above. But definitively, feel free to send follow-up PR(s) when this one lands, if you've got ideas for a better solution!

PR Close angular#22498

angular-automatic-lock-bot · 2019-09-13T09:50:59Z

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 Feb 28, 2018

pkozlowski-opensource requested a review from mhevery February 28, 2018 16:47

pkozlowski-opensource added target: major This PR is targeted for the next major release comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 28, 2018

vicb reviewed Feb 28, 2018

View reviewed changes

mhevery reviewed Feb 28, 2018

View reviewed changes

chuckjaz approved these changes Feb 28, 2018

View reviewed changes

vicb suggested changes Feb 28, 2018

View reviewed changes

pkozlowski-opensource force-pushed the ivy_ng_project_as branch from 5a57866 to 81f375c Compare March 1, 2018 10:58

mhevery approved these changes Mar 1, 2018

View reviewed changes

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 1, 2018

vicb reviewed Mar 1, 2018

View reviewed changes

vicb approved these changes Mar 1, 2018

View reviewed changes

pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Mar 1, 2018

pkozlowski-opensource force-pushed the ivy_ng_project_as branch 2 times, most recently from 6cd602f to 6f5e7a5 Compare March 5, 2018 09:38

pkozlowski-opensource added 3 commits March 6, 2018 11:11

feat(ivy): add support for the ngProjectAs attribute

aca7b4b

fixup! feat(ivy): add support for the ngProjectAs attribute

6b512cf

fixup! feat(ivy): add support for the ngProjectAs attribute

2194b45

pkozlowski-opensource force-pushed the ivy_ng_project_as branch from 6f5e7a5 to 2194b45 Compare March 6, 2018 10:12

alexeagle closed this in 2c75acc Mar 6, 2018

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(ivy): add support for the ngProjectAs attribute (angular#22498)

c3e49e4

PR Close angular#22498

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ivy): add support for the ngProjectAs attribute #22498

feat(ivy): add support for the ngProjectAs attribute #22498

pkozlowski-opensource commented Feb 28, 2018

vicb Feb 28, 2018

pkozlowski-opensource Mar 1, 2018

vicb Feb 28, 2018

pkozlowski-opensource Mar 1, 2018

vicb Feb 28, 2018

pkozlowski-opensource Mar 1, 2018

vicb Feb 28, 2018

vicb Feb 28, 2018

pkozlowski-opensource Mar 1, 2018

pkozlowski-opensource Mar 1, 2018

vicb Feb 28, 2018

pkozlowski-opensource Mar 1, 2018

mhevery Feb 28, 2018

vicb Feb 28, 2018

pkozlowski-opensource Mar 1, 2018

pkozlowski-opensource Mar 1, 2018

chuckjaz left a comment

vicb left a comment

pkozlowski-opensource commented Mar 1, 2018

vicb Mar 1, 2018

vicb Mar 1, 2018

vicb Mar 1, 2018

vicb Mar 1, 2018

vicb left a comment

JoostK commented Mar 1, 2018

pkozlowski-opensource commented Mar 1, 2018

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

feat(ivy): add support for the ngProjectAs attribute #22498

feat(ivy): add support for the ngProjectAs attribute #22498

Conversation

pkozlowski-opensource commented Feb 28, 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

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

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

chuckjaz left a comment

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

pkozlowski-opensource commented Mar 1, 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

vicb left a comment

Choose a reason for hiding this comment

JoostK commented Mar 1, 2018

pkozlowski-opensource commented Mar 1, 2018

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