refactor(ivy): misc #23351
refactor(ivy): misc #23351
Conversation
|
You can preview 770a777 at https://pr23351-770a777.ngbuilds.io/. |
| /** Whether or not this node is a component */ | ||
| Component = 0b001, | ||
| /** The number of directives on this node is encoded on the least significant bits */ | ||
| NbOfDirectivesMask = 0b00000000000000000000111111111111, |
There was a problem hiding this comment.
How about DirSizeMask? Find Nb kind of hard to read.
There was a problem hiding this comment.
DirSize is confusing IMO ? It could mean that directive have size.
There was a problem hiding this comment.
If we keep number, can we write it out then as number? Nb took me a second to understand since it's not the normal abbreviation.
There was a problem hiding this comment.
How about just DirectiveCountMask?
There was a problem hiding this comment.
Ah, count. That is clearer than size.
| /** Mask to get the number of directives on this node */ | ||
| SIZE_MASK = 0b00000000000000000001111111111110 | ||
| /** The index of the first directive on this node is encoded on the most significant bits */ | ||
| IndexOfFirstDirectiveShift = 13, |
There was a problem hiding this comment.
How about DirIndexShift? This is kind of long and hard to read.
There was a problem hiding this comment.
Same kind or remark.
More explicit does not hurt
There was a problem hiding this comment.
I'm just concerned that the trade-off is readability, but I don't feel strongly
There was a problem hiding this comment.
how about DirectiveStartingIndexShift
| ngDevMode && assertNotEqual( | ||
| previousOrParentNode.tNode !.flags & TNodeFlags.NbOfDirectivesMask, | ||
| TNodeFlags.NbOfDirectivesMask, 'Reached the max number of directives'); | ||
| previousOrParentNode.tNode !.flags++; |
There was a problem hiding this comment.
Are you saving the first index? Looks like it's deleted here, which is probably why the tests are failing.
We should only be adding to the size this way if the first directive index has already been saved by a previous directive.
vicb
added
action: review
|
@kara PTAL |
|
You can preview 5247915 at https://pr23351-5247915.ngbuilds.io/. |
|
You can preview d824e05 at https://pr23351-d824e05.ngbuilds.io/. |
|
You can preview e8b265a at https://pr23351-e8b265a.ngbuilds.io/. |
|
You can preview f39ed82 at https://pr23351-f39ed82.ngbuilds.io/. |
vicb
added
action: merge
packages/core/src/render3/di.ts
Outdated
| // Get the definition for the directive at this index and, if it is injectable (diPublic), | ||
| // and matches the given token, return the directive instance. | ||
| const directiveDef = defs[i] as DirectiveDef<any>; | ||
| if (directiveDef.diPublic && directiveDef.type == token) { | ||
| if (directiveDef.type == token && directiveDef.diPublic) { |
There was a problem hiding this comment.
Shouldn't this use triple-equals comparison? It is now different from getIdxOfMatchingDirective in render3/query.ts
packages/core/src/render3/query.ts
Outdated
| @@ -196,10 +196,12 @@ function getIdxOfMatchingSelector(tNode: TNode, selector: string): number|null { | |||
| function geIdxOfMatchingDirective(node: LNode, type: Type<any>): number|null { | |||
There was a problem hiding this comment.
This should probably be named getIdxOfMatchingDirective
|
You can preview 6f13bbc at https://pr23351-6f13bbc.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. |
No description provided.