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

Dynamic components aren't hydrated when root component is used as an anchor #51157

Closed
devknoll opened this issue Jul 24, 2023 · 11 comments
Closed
Assignees
Labels
area: core Issues related to the framework runtime core: hydration P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: has PR
Milestone

Comments

@devknoll
Copy link
Contributor

Which @angular/* package(s) are the source of the bug?

platform-browser

Is this a regression?

No

Description

As in the linked reproduction, I'm dynamically creating a component like so:

import('./foo/foo.component').then((m) => {
  this.vcr.createComponent(m.FooComponent);
});

The component correctly appears in the SSR output. However, Angular does not appear to hydrate the component. Instead, it creates a new instance, and leaves the existing copy in the DOM dehydrated (and thus, broken), even after hydration completes.

Please provide a link to a minimal reproduction of the bug

https://codesandbox.io/p/sandbox/unruffled-dawn-dngll7

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in (run ng version)

No response

Anything else?

No response

@AndrewKushnir AndrewKushnir self-assigned this Jul 24, 2023
@AndrewKushnir AndrewKushnir added area: core Issues related to the framework runtime core: hydration labels Jul 24, 2023
@ngbot ngbot bot added this to the needsTriage milestone Jul 24, 2023
@JeanMeche
Copy link
Member

JeanMeche commented Jul 24, 2023

ViewContainerRef.createComponent adds the component as sibling of the curremnt component. This means when you use ViewContainerRef.createComponent in the root component, the component is created outside the Angular app (It's created outside the <app-root></app-root>.

There is no issue when you do the samething in any other compoent !

@AndrewKushnir AndrewKushnir added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Jul 24, 2023
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 24, 2023
@AndrewKushnir
Copy link
Contributor

I've performed an initial investigation and can confirm what @JeanMeche mentioned above: the problem only happens in a root components when inject(ViewContainerRef) is used, thus transforming a value the first slot of a root LView to LContainer (from a component's LView). Related code: https://github.com/angular/angular/blob/main/packages/core/src/hydration/annotate.ts#L101-L122. The current logic basically skips serialization of all views within a root view container. The correct logic should handle such a case and go over all views within a view container recursively (since containers might potentially have nested view containers as well).

There is also 1 limitation that we would likely need to add: such view containers can not have embedded views in them (i.e. views create via ViewContainerRef.createEmbeddedView), since there is no host element there where we can place ngh id to refer to serialized info. There is a comment node (acting as a host node) available for those views, but the current hydration logic can't use those nodes as a container for ngh id info yet (we'd to support those cases in the future though for partial per-view hydration).

@AndrewKushnir AndrewKushnir changed the title Dynamic components aren't hydrated Dynamic components aren't hydrated when root component is used as an anchor Jul 24, 2023
@AndrewKushnir
Copy link
Contributor

Note (to myself): here is a new TestBed test (can be added to the packages/platform-server/test/hydration_spec.ts) that reproduces the issue:

      it('should hydrate dynamically created components using root component as an anchor', async () => {
        @Component({
          standalone: true,
          imports: [CommonModule],
          selector: 'dynamic',
          template: `
            <span>This is a content of a dynamic component.</span>
            <b *ngIf="isServer">This is a SERVER-ONLY content</b>
            <i *ngIf="!isServer">This is a CLIENT-ONLY content</i>
          `,
        })
        class DynamicComponent {
          isServer = isPlatformServer(inject(PLATFORM_ID));
        }

        @Component({
          standalone: true,
          selector: 'app',
          template: `
            <main>Hi! This is the main content.</main>
          `,
        })
        class SimpleComponent {
          vcr = inject(ViewContainerRef);

          ngAfterViewInit() {
            const compRef = this.vcr.createComponent(DynamicComponent);
            compRef.changeDetectorRef.detectChanges();
          }
        }

        const html = await ssr(SimpleComponent);
        let ssrContents = getAppContents(html);

        expect(ssrContents).toContain('<app ngh');

        resetTViewsFor(SimpleComponent, DynamicComponent);

        const appRef = await hydrate(html, SimpleComponent);
        const compRef = getComponentRef<SimpleComponent>(appRef);
        appRef.tick();

        ssrContents = stripExcessiveSpaces(stripUtilAttributes(ssrContents, false));

        // We expect to see SERVER content, but not CLIENT.
        expect(ssrContents).not.toContain('<i>This is a CLIENT-ONLY content</i>');
        expect(ssrContents).toContain('<b>This is a SERVER-ONLY content</b>');

        const clientRootNode = compRef.location.nativeElement;

        await whenStable(appRef);

        const clientContents =
            stripExcessiveSpaces(stripUtilAttributes(clientRootNode.outerHTML, false));

        // After the cleanup, we expect to see CLIENT content, but not SERVER.
        expect(clientContents).toContain('<i>This is a CLIENT-ONLY content</i>');
        expect(clientContents).not.toContain('<b>This is a SERVER-ONLY content</b>');
      });

@hiepxanh
Copy link
Contributor

hiepxanh commented Aug 1, 2023

Hi, I'm using ngx-dynamic-hooks which is have the same logic. Which have the same issue because of the recreation of angular component which cause by hydration logic. Can I suggest some kind of attribute mark which notice that the Hydration should do nothing with that component?

p/s: I'm using old day workaround to solve this problem MTobisch/ngx-dynamic-hooks#34 (comment)

AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Aug 2, 2023
For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Aug 2, 2023
For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.
@AndrewKushnir
Copy link
Contributor

@hiepxanh I've created a fix for the problem reported in this ticket, see PR #51247. You can test (locally) if this fix also resolves the problem that you've mentioned by using build artifacts from the PR. You can try updating your package.json file and change the reference to the @angular/core package and running npm install --force:

"dependencies": {
  ...
  "@angular/core": "https://output.circle-artifacts.com/output/job/5457f352-d3e1-4417-943e-6a341dac8930/artifacts/0/angular/core-pr51247-5386d318c4.tgz",
  ...
}

Note: this archive is available only temporarily, please do not use it in production builds.

@hiepxanh
Copy link
Contributor

hiepxanh commented Aug 2, 2023

@AndrewKushnir Oh my god! I'm really lucky to meet you, sir. Every time I get stuck in a hard issue, you just appear like a god. It has been 1 year since the last issue #46473 (comment) and we meet each other again. Thank you so much for this support. It really works and the performance has improved a lot.
This will be really useful for drag and drop or some Angular builder.
Since Angular 2 beta and Angular JS, I never lost faith among a lot of JS frameworks. And after 10 years of working with Angular, my knowledge is still usable in this version.

Thank you so much, it works! I can confirm it worked!
I hope this fix can land in the next version.

Before change, there is a flick which lost and generate the HTML again.

image

After change, it stable no flick:
image

@AndrewKushnir
Copy link
Contributor

@hiepxanh thanks for your kind words!

You did a great job tracking down the root cause of the problem to this ticket 👍

Great to hear that the fix resolves the issue in your application and you see performance improvements. The fix would go though the regular process (code review, testing and merging) and will be available in one of the upcoming Angular weekly releases. I'll keep this thread updated once the fix is released.

Thank you.

AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Aug 2, 2023
For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Aug 4, 2023
For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Aug 4, 2023
For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.
dylhunn pushed a commit that referenced this issue Aug 7, 2023
…51247)

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves #51157.

PR Close #51247
@dylhunn dylhunn closed this as completed in 55965cb Aug 7, 2023
@AndrewKushnir
Copy link
Contributor

@hiepxanh just wanted to let you know that Angular v16.2.0 is out (available on NPM) and this fix is included into the release. Thank you.

@hiepxanh
Copy link
Contributor

I can confirm it work in the codesandbox.io https://codesandbox.io/p/sandbox/relaxed-feather-27fv8y with angular 16.2.0 version. Thank you for your support @AndrewKushnir

@hiepxanh
Copy link
Contributor

@AndrewKushnir thank you so much sir. But I think it lead to other issue. No bug no life, I think so.
Anyway can you take a look on this issue?
#51318

thomasturrell pushed a commit to thomasturrell/angular that referenced this issue Aug 29, 2023
…ngular#51247)

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.

PR Close angular#51247
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 29, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
…ngular#51247)

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.

PR Close angular#51247
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime core: hydration P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: has PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants