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

feat(ivy): namespaced attributes added to output instructions #24386

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@benlesh
Contributor

benlesh commented Jun 8, 2018

NOTE: This does NOT add parsing of namespaced attributes

  • Adds AttributeMarker for namespaced attributes
  • Adds test for namespaced attributes
  • Updates AttributeMarker enum to use CamelCase, and not UPPER_CASE names.
  • Does NOT add changes to compiler to process templates that have namespaced attributes, yet. (see #24426),
    • Adds xit test in packages/compiler/test/render3/r3_compiler_compliance_spec.ts for update

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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 current behavior?

Currently there is no output instruction handling for attributes with namespacing on elements

Issue Number: N/A

What is the new behavior?

element and elementStart will support arrays of attributes that allow for namespaced elements to be passed in like so:

[AttributeMarker.NamespaceURI, 'http://someuri', 'prefix:name', 'value']

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Related to #23899

const namespaceURI = attrs[i + 1] as string;
const attrName = attrs[i + 2] as string;
const attrVal = attrs[i + 3] as string;
i += 2;

This comment has been minimized.

@vicb

vicb Jun 8, 2018

Contributor

nit: might be less confusing to NOT increment in the for(i += 2) and increment in each case ? I had to compute 2+2 to make this you were right ;)

This comment has been minimized.

@benlesh

benlesh Jun 8, 2018

Contributor

done

step = 1;
}
if (attrName === AttributeMarker.NamespaceURI) {
step = 4;

This comment has been minimized.

@vicb

vicb Jun 8, 2018

Contributor

looks wrong to me as we never reset to 2 ?
again I think i += step is confusing. Remove it from the for() and increment i in an if/else/else
Make sure there is a test for that case.
Please also add a test for SelectOnly which seems to be affected.

@@ -261,7 +261,7 @@ export function injectAttribute(attrNameToInject: string): string|undefined {
if (attrs) {
for (let i = 0; i < attrs.length; i = i + 2) {
const attrName = attrs[i];
if (attrName === AttributeMarker.SELECT_ONLY) break;
if (attrName === AttributeMarker.SelectOnly) break;

This comment has been minimized.

@vicb

vicb Jun 8, 2018

Contributor

note: I would have done a first refactor commit with the name change and the actual interesting change in a subsequent commit (no need to change now)

This comment has been minimized.

@benlesh

benlesh Jun 8, 2018

Contributor

True.... I was all worried about the "fixup" business :P

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 8, 2018

@vicb

vicb suggested changes Jun 8, 2018 edited

  • see inline comments
  • please add compiler_canonical tests so that we know what to implement compiler side - also update status.md with a link to those tests.
@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 8, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Jun 11, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: lint" is failing
    pending status "code-review/pullapprove" is pending
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 11, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 11, 2018

@Component({
selector: 'my-component',
template: \`<div xmlns:foo="http://someurl/foo" class="my-app" foo:bar="baz" title="Hello">Hello <b>World</b>!</div>\`

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

There should probably be more than 1 ns attribute here (to make it clear that 0 is not a marker used only once in the attr list).

@@ -1466,11 +1484,12 @@ function generateInitialInputs(
const attrs = tNode.attrs !;
for (let i = 0; i < attrs.length; i += 2) {
const attrName = attrs[i];
const first = attrs[i];
const attrName = first === AttributeMarker.NamespaceURI ? attrs[i += 2] : first;

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

does not sound right ?
either we should discard ns attributes totally or deal with the ns ?

if (selectOnlyMarkerIdx > -1 && attrIndexInNode > selectOnlyMarkerIdx) {
nodeAttrValue = '';
} else if (maybeAttrName === AttributeMarker.NamespaceURI) {
nodeAttrValue = nodeAttrs[attrIndexInNode + 2] as string;

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

does not sound right ?
either we should discard ns attributes totally or deal with the ns ?

// title="Hello"
'title',
'Hello',
// foo:bar="baz"

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

wrong comment

This comment has been minimized.

@benlesh

benlesh Jun 11, 2018

Contributor

fixed

// The template should look like this (where IDENT is a wild card for an identifier):
const template = `
const $c1$ = ['class', 'my-app', 0, 'http://someuri/foo', 'foo:bar', 'baz', 'title', 'Hello'];

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

this spec needs be updated to match the core test

This comment has been minimized.

@benlesh

benlesh Jun 11, 2018

Contributor

done

@vicb

see inline comments

@@ -147,6 +147,7 @@ The goal is for the `@Component` (and friends) to be the compiler of template. S
| `<div style="literal">` | ✅ | ✅ | ✅ |
| `<div [style]="exp">` | ❌ | ❌ | ❌ |
| `<div [style.foo]="exp">` | ✅ | ✅ | ❌ |
| `<div xmlns:foo="url" foo:bar="baz">` | ✅ | ✅ | ❌ |

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

please add a link to this PR and describe what is left to do compiler in the header of the GH PR. (check ng-template below l 160)

This comment has been minimized.

@benlesh

benlesh Jun 11, 2018

Contributor

Done

// The template should look like this (where IDENT is a wild card for an identifier):
const template = `
const $c1$ = ['class', 'my-app', 0, 'http://someuri/foo', 'foo:bar', 'baz', 'title', 'Hello', 0, 'http://someuri/foo', 'foo:qux', 'quacks'];

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

should be http://someurl/foo ?

@Component({
selector: 'my-component',
template:
`<div class="my-app" foo:bar="baz" title="Hello" foo:qux="quacks">Hello <b>World</b>!</div>`

This comment has been minimized.

@vicb

vicb Jun 11, 2018

Contributor

does not match the compiler test. They should be the same.
If it is not possible for now there should be a clear description in the PR header why and what is left to be done.

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 11, 2018

@vicb

vicb approved these changes Jun 11, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 13, 2018

@mhevery mhevery closed this in 82c5313 Jun 13, 2018

@mhevery mhevery referenced this pull request Jun 14, 2018

Closed

feat(ivy): SVG handling added #23899

2 of 3 tasks complete

@benlesh benlesh deleted the benlesh:ivy-attr-ns branch Jul 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment