Skip to content
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

Fw 1559 - more cases #33493

Conversation

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Oct 30, 2019

This is a version of #33457 that has more tests and handles more cases.

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:fw-1559-pawel-take branch from 7efe67a to 87d23cc Oct 30, 2019
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member Author

pkozlowski-opensource commented Oct 30, 2019

core/todo/bundle increased by 126 bytes.

This is mostly due to the fact that we are starting to have code duplication with

function applyToElementOrContainer(
action: WalkTNodeTreeAction, renderer: Renderer3, parent: RElement | null,
lNodeToHandle: RNode | LContainer | LView, beforeNode?: RNode | null) {
// If this slot was allocated for a text node dynamically created by i18n, the text node itself
// won't be created until i18nApply() in the update block, so this node should be skipped.
// For more info, see "ICU expressions should work inside an ngTemplateOutlet inside an ngFor"
// in `i18n_spec.ts`.
if (lNodeToHandle != null) {
let lContainer: LContainer|undefined;
let isComponent = false;
// We are expecting an RNode, but in the case of a component or LContainer the `RNode` is
// wrapped in an array which needs to be unwrapped. We need to know if it is a component and if
// it has LContainer so that we can process all of those cases appropriately.
if (isLContainer(lNodeToHandle)) {
lContainer = lNodeToHandle;
} else if (isLView(lNodeToHandle)) {
isComponent = true;
ngDevMode && assertDefined(lNodeToHandle[HOST], 'HOST must be defined for a component LView');
lNodeToHandle = lNodeToHandle[HOST] !;
}
const rNode: RNode = unwrapRNode(lNodeToHandle);
ngDevMode && !isProceduralRenderer(renderer) && assertDomNode(rNode);
if (action === WalkTNodeTreeAction.Create && parent !== null) {
if (beforeNode == null) {
nativeAppendChild(renderer, parent, rNode);
} else {
nativeInsertBefore(renderer, parent, rNode, beforeNode || null);
}
} else if (action === WalkTNodeTreeAction.Insert && parent !== null) {
nativeInsertBefore(renderer, parent, rNode, beforeNode || null);
} else if (action === WalkTNodeTreeAction.Detach) {
nativeRemoveNode(renderer, rNode, isComponent);
} else if (action === WalkTNodeTreeAction.Destroy) {
ngDevMode && ngDevMode.rendererDestroyNode++;
(renderer as ProceduralRenderer3).destroyNode !(rNode);
}
if (lContainer != null) {
applyContainer(renderer, action, lContainer, parent, beforeNode);
}
}
}

In the essence both code-paths are solving the same problem - give me a set of root native nodes. We just happen to do it for different purposes, but the returned value is the same.

We need to remove this code duplication in a follow-up PR(s)

} else {
// ViewEngine seems to produce very different DOM structure as compared to ivy
// when it comes to ICU containers - this needs more investigation / fix.
expect(rootNodes.length).toBe(7);

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Oct 30, 2019

Author Member

@AndrewKushnir it seems like VE and ivy are generating very different DOM when it comes to ICU expressions. Not sure if this is intended and if not, are we tracking it anywhere?

This comment has been minimized.

Copy link
@mhevery

mhevery Oct 30, 2019

Member

This would be expected for ICU expressions.

@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Oct 30, 2019
@pkozlowski-opensource pkozlowski-opensource requested a review from angular/fw-core as a code owner Oct 30, 2019
@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Oct 30, 2019

AndrewKushnir added a commit that referenced this pull request Oct 30, 2019
AndrewKushnir added a commit that referenced this pull request Oct 30, 2019
AndrewKushnir added a commit that referenced this pull request Oct 30, 2019
AndrewKushnir added a commit that referenced this pull request Oct 30, 2019
@pkozlowski-opensource pkozlowski-opensource referenced this pull request Nov 5, 2019
5 of 14 tasks complete
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.