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

lazy-in-for #26

Closed
wants to merge 1 commit into from
Closed

Conversation

janstadt
Copy link

@janstadt janstadt commented Oct 7, 2019

Workaround to make the dynamic component work correctly in ngFor. Still have to test this out with requests that take longer than others.

@tomastrajan
Copy link
Member

@janstadt amazing thank you! please let me know ASAP and I will merge and release it!

@@ -43,7 +43,6 @@ export class LazyElementDynamicDirective implements OnInit {
}

const host = (this.template as any)._def.element.template.nodes[0].element;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't it help to clone the element so that when we change the tag, we're changing it only for this one particular element and not every similar element ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that would be helpful, although im not 100% sure thats the issue and i wasnt completely sure how to create a new TemplateRef from scratch. Its abstract so i cant instantiate a new one and when its injected im not sure how angular is handling that. The TemplateRef seems to be the key though.

So from what i am following in the code, the reason why you've decided to swap out the tag name is because then that element will still have the inputs/outputs that are needed for the web component to bind appropriately to the container component. Instead of changing the tag name, could we just add another html element to the dom and loop through the current elements attributes to get all the bindings?

FYI, this PR didnt work completely. I added more test elements (it worked with 2) to the array but still have the race condition. Will keep looking into a solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janstadt would try to prevent introducing such logic as manual bindings behind the scene cuz that would be prolly very fragile... So what about the case of looping over X elements which are the same with same bindings, works ? Also for cloning I only thought about the last .element object not the whole this.template ? Could help ?

Copy link
Author

@janstadt janstadt Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomastrajan im not sure thats possible. Im running out of ideas. I might be able to just use the other non dynamic loader with a bunch of if checks based on the user response for which modules they have access to. Clearly not optimal but would get me past this issue. It seems like there should be an easier way to do this but it doesnt seem that straight forward. I did try to create a new element like this:

const axLazy = this.template.elementRef.nativeElement.nextSibling;
var newElement = document.createElement(this.tag);
        
        for(let i = 0; i < axLazy.attributes.length; i++) {
            const item = axLazy.attributes.item(i);
            newElement.setAttribute(item.nodeName, item.nodeValue);
        }

        this.renderer.removeChild(this.template.elementRef.nativeElement.parentElement, axLazy);
        this.renderer.appendChild(this.template.elementRef.nativeElement.parentElement, newElement);

At one point nextSibling was valued as the actual ax-lazy-element but now its always null due to the fact that the template hasnt been completely rendered at the time of the check which is unfortunate. Not sure how i was able to get it to be valued but it must have been some weird race condition yet again. Let me know if you are able to get anything working.

Thanks!

@tomastrajan
Copy link
Member

Was solved in other PR, thx for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants