-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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): support host attribute and property bindings #22334
Conversation
You can preview c538e4a at https://pr22334-c538e4a.ngbuilds.io/. |
query.first ? temporary.key(o.literal(0)) : temporary); | ||
const updateDirective = o.variable(CONTEXT_NAME) | ||
.prop(query.propertyName) | ||
.set(query.first ? temporary.key(o.literal(0)) : temporary); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as previous PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it is the same code :)
function createHostAttributesArray( | ||
directiveMetadata: CompileDirectiveMetadata, outputCtx: OutputContext): o.Expression|null { | ||
const values: o.Expression[] = []; | ||
const attributes = directiveMetadata.hostAttributes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.getOwnPropertyNames()
query.first ? temporary().key(o.literal(0)) : temporary()); | ||
const updateDirective = getDirectiveMemory.key(o.literal(0, o.INFERRED_TYPE)) | ||
.prop(query.propertyName) | ||
.set(query.first ? temporary().key(o.literal(0)) : temporary()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here also
c538e4a
to
656747e
Compare
You can preview 656747e at https://pr22334-656747e.ngbuilds.io/. |
656747e
to
fd85229
Compare
You can preview fd85229 at https://pr22334-fd85229.ngbuilds.io/. |
Hi @chuckjaz! This PR has merge conflicts due to recent upstream merges. |
fd85229
to
169b939
Compare
const definitionMapValues: {key: string, quoted: boolean, value: o.Expression}[] = []; | ||
|
||
const field = (key: string, value: o.Expression | null) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if 2 is enough for a fieldBuilderBuilder()
? May be if you anticipate more usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really but it is more consistent with the one above.
const functionName = | ||
typeName && bindingName ? `${typeName}_${bindingName}_HostBindingHandler` : null; | ||
const handler = o.fn( | ||
[new o.FnParam('event', o.DYNAMIC_TYPE)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should 'b' and 'event' be consts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
event
is a parameter and 'b' is the context which is generated in the convertActionBinding
which is not part of this change.
dirMeta: CompileDirectiveSummary, elementSelector: string, | ||
sourceSpan: ParseSourceSpan): BoundElementPropertyAst[]|null { | ||
const boundProps = this.createBoundHostProperties(dirMeta, sourceSpan); | ||
return boundProps && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: return boundProps ? map : null
(only style change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the coalescing pattern. It is sad that JavaScript doesn't have a real one but for object references && works.
@@ -243,5 +251,9 @@ export function compile( | |||
/* referenceFilter */ undefined, | |||
/* importFilter */ e => e.moduleName != null && e.moduleName.startsWith('/app')); | |||
|
|||
if (errors.length) { | |||
throw new Error('Unexpected errors:' + errors.map(e => e.toString).join(', ')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is map(e => e.toString)
required here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't emit the right result, the fixed, toString() call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits
You can preview 169b939 at https://pr22334-169b939.ngbuilds.io/. |
You can preview a5b7c9d at https://pr22334-a5b7c9d.ngbuilds.io/. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Ivy compiler does not support host bindings.
What is the new behavior?
Ivy compiler supports host bindings.
Does this PR introduce a breaking change?