Skip to content

refactor(platform-browser): replace isPlatformServer with ngServerMode#59496

Closed
arturovt wants to merge 1 commit intoangular:mainfrom
arturovt:refactor/platform-browser-renderer-ngservermode
Closed

refactor(platform-browser): replace isPlatformServer with ngServerMode#59496
arturovt wants to merge 1 commit intoangular:mainfrom
arturovt:refactor/platform-browser-renderer-ngservermode

Conversation

@arturovt
Copy link
Copy Markdown
Contributor

@arturovt arturovt commented Jan 13, 2025

In this commit, we switch from using the isPlatformServer runtime call to the ngServerMode.

Note: constructors haven't been touched in order to prevent any breaking changes for the public API.

@pullapprove pullapprove bot requested a review from alxhub January 13, 2025 11:28
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Jan 13, 2025
@ngbot ngbot bot added this to the Backlog milestone Jan 13, 2025
@arturovt arturovt force-pushed the refactor/platform-browser-renderer-ngservermode branch from 60a49ba to 1c59661 Compare January 13, 2025 19:02
@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Mar 5, 2025

@arturovt Can you resolve the conflicts here and take a look at the failing tests? I suspect they are related to this change.

@JeanMeche JeanMeche force-pushed the refactor/platform-browser-renderer-ngservermode branch from 1c59661 to be02523 Compare March 5, 2025 18:05
@pkozlowski-opensource
Copy link
Copy Markdown
Member

pkozlowski-opensource commented Mar 5, 2025

Shouldn't we drop isPlatformServer utility if we don't intend to use it from now on?

@JeanMeche
Copy link
Copy Markdown
Member

JeanMeche commented Apr 2, 2025

isPlatformServer is public while ngServerMode isn't.

thePunderWoman
thePunderWoman previously approved these changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

I think we can move forward with this. @arturovt Can you do the cleanup on this?

@thePunderWoman thePunderWoman added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Apr 2, 2025
@thePunderWoman thePunderWoman removed the request for review from alxhub April 2, 2025 11:51
@JeanMeche JeanMeche force-pushed the refactor/platform-browser-renderer-ngservermode branch from be02523 to d03c92e Compare April 2, 2025 11:53
@thePunderWoman thePunderWoman force-pushed the refactor/platform-browser-renderer-ngservermode branch from d03c92e to 7916a23 Compare April 2, 2025 12:48
@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Apr 2, 2025

@arturovt Unfortunately it looks like there's some legit test failures.

@arturovt arturovt force-pushed the refactor/platform-browser-renderer-ngservermode branch from 7916a23 to 1ae2bf5 Compare April 2, 2025 19:49
@angular-robot angular-robot bot requested a review from thePunderWoman April 2, 2025 19:49
@arturovt arturovt force-pushed the refactor/platform-browser-renderer-ngservermode branch from 1ae2bf5 to f394c49 Compare May 21, 2025 14:00
@thePunderWoman thePunderWoman dismissed their stale review May 21, 2025 14:37

This continues to have legitimate test failures. We'll need to re-review when this is genuinely green.

@arturovt
refactor(platform-browser): replace isPlatformServer with `ngServer…
89a1450 
…Mode`

In this commit, we switch from using the `isPlatformServer` runtime call to the `ngServerMode`.

Note: constructors haven't been touched in order to prevent any breaking changes for the public API.
@arturovt arturovt force-pushed the refactor/platform-browser-renderer-ngservermode branch from f394c49 to 89a1450 Compare May 21, 2025 16:45
@thePunderWoman thePunderWoman 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 May 22, 2025
thePunderWoman
thePunderWoman reviewed May 22, 2025
View reviewed changes
packages/platform-browser/src/dom/dom_renderer.ts

if (this.platformIsServer && type.encapsulation === ViewEncapsulation.ShadowDom) {
if (
typeof ngServerMode !== 'undefined' &&
Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman May 22, 2025

Choose a reason for hiding this comment

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

Why not just use this.platformIsServer assigned in the constructor here?

Copy link
Copy Markdown
Member

@JeanMeche JeanMeche May 22, 2025

Choose a reason for hiding this comment

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

I don't think the optimizer would be able to infer the value of this.platformIsServer as false and remove the condition.

Copy link
Copy Markdown
Contributor Author

@arturovt arturovt May 22, 2025

Choose a reason for hiding this comment

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

Exactly.

thePunderWoman
thePunderWoman approved these changes May 22, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

@thePunderWoman thePunderWoman added action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels May 22, 2025
thePunderWoman pushed a commit that referenced this pull request May 26, 2025
@arturovt @thePunderWoman
refactor(platform-browser): replace isPlatformServer with `ngServer…
833c69b 
…Mode` (#59496)

In this commit, we switch from using the `isPlatformServer` runtime call to the `ngServerMode`.

Note: constructors haven't been touched in order to prevent any breaking changes for the public API.

PR Close #59496
@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented May 26, 2025

This PR was merged into the repository by commit 9a9511a.

The changes were merged into the following branches: main, 20.0.x

@arturovt arturovt deleted the refactor/platform-browser-renderer-ngservermode branch May 26, 2025 11:59
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Jun 26, 2025

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 Jun 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JeanMeche JeanMeche JeanMeche left review comments

@thePunderWoman thePunderWoman thePunderWoman approved these changes

+1 more reviewer

@rhutchison rhutchison rhutchison approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: rc This PR is targeted for the next release-candidate

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

5 participants

@arturovt @thePunderWoman @pkozlowski-opensource @JeanMeche @rhutchison