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

fix(platform-browser): reuse server generated component styles #48253

Closed
wants to merge 8 commits into from

Conversation

alan-agius4
Copy link
Contributor

fix(core): generate consistent component IDs

Prior to this change the component IDs where generated based on a counter. This is problematic as there is no guarantee that a component will get the same ID that was assigned on the server when generated on the client side.

This is paramount to be able to re-use the component styles generated on the server.

fix(platform-browser): reuse server generated component styles

Prior to this change component styles generated on the server where removed prior to the client side component being rendered and attached it's own styles. In some cases this caused flickering. To mitigate this initialNavigation: enabledBlocking' was introduced which allowed the remove of server styles to be defer to a latter stage when the application has finished initialization.

This commit changes the need for this, by not removing the server generated component styles and reuse them for client side rendering.

refactor(platform-browser): remove internal TRANSITION_ID token

This was a mirror copy of the APP_ID token.

@alan-agius4 alan-agius4 added area: core Issues related to the framework runtime area: server Issues related to server-side rendering target: patch This PR is targeted for the next patch release labels Nov 28, 2022
@ngbot ngbot bot modified the milestone: Backlog Nov 28, 2022
@alan-agius4 alan-agius4 self-assigned this Nov 28, 2022
@alan-agius4 alan-agius4 force-pushed the re-use-style branch 11 times, most recently from ca1ca9b to c1b61a5 Compare November 28, 2022 18:45
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@alan-agius4 thanks for the PR 👍

The approach looks promising, I've just left a few minor comments/questions.

integration/platform-server/e2e/helloworld-spec.ts Outdated Show resolved Hide resolved
packages/platform-browser/test/browser/bootstrap_spec.ts Outdated Show resolved Hide resolved
packages/platform-browser/src/dom/shared_styles_host.ts Outdated Show resolved Hide resolved
packages/platform-browser/src/dom/shared_styles_host.ts Outdated Show resolved Hide resolved
packages/platform-browser/src/dom/shared_styles_host.ts Outdated Show resolved Hide resolved
@alan-agius4 alan-agius4 marked this pull request as ready for review November 29, 2022 10:23
@alan-agius4 alan-agius4 force-pushed the re-use-style branch 3 times, most recently from 29c2b40 to fee45a5 Compare November 30, 2022 08:07
@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 8, 2023
Prior to this change the component IDs where generated based on a counter. This is problematic as there is no guarantee that a component will get the same ID that was assigned on the server when generated on the client side.

This is paramount to be able to re-use the component styles generated on the server.
Prior to this change component styles generated on the server where removed prior to the client side component being rendered and attached it's own styles. In some cases this caused flickering. To mitigate this `initialNavigation: enabledBlocking'` was introduced which allowed the remove of server styles to be defer to a latter stage when the application has finished initialization.

This commit changes the need for this, by not removing the server generated component styles and reuse them for client side rendering.
This was a mirror copy of the `APP_ID` token.
@alan-agius4 alan-agius4 removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 9, 2023
@alan-agius4 alan-agius4 force-pushed the re-use-style branch 2 times, most recently from 5acdb8e to 3b536f4 Compare March 9, 2023 08:34
@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Mar 9, 2023

@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 9, 2023
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@alan-agius4 looks great, thanks for working on this change! 👍

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 9, 2023
@AndrewKushnir
Copy link
Contributor

(adding the "merge" label after an LGTM from @alan-agius4)

@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 86fc4d3.

jessicajaniuk pushed a commit that referenced this pull request Mar 9, 2023
Prior to this change component styles generated on the server where removed prior to the client side component being rendered and attached it's own styles. In some cases this caused flickering. To mitigate this `initialNavigation: enabledBlocking'` was introduced which allowed the remove of server styles to be defer to a latter stage when the application has finished initialization.

This commit changes the need for this, by not removing the server generated component styles and reuse them for client side rendering.

PR Close #48253
jessicajaniuk pushed a commit that referenced this pull request Mar 9, 2023
…8253)

This was a mirror copy of the `APP_ID` token.

PR Close #48253
@alan-agius4 alan-agius4 deleted the re-use-style branch March 9, 2023 18:34
@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 Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime area: server Issues related to server-side rendering core: stylesheets target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants