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.

Sign up for GitHub

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

Jump to bottom

fix(compiler): apply style on :host attribudes in prod builds. #49118

Closed
wants to merge 1 commit into from
Closed

fix(compiler): apply style on :host attribudes in prod builds. #49118

wants to merge 1 commit into from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented Feb 16, 2023 •
edited

In prod builds, selectors are optimized and spaces a removed. #48558 introduced a regression on selectors without spaces. This commit fixes this.

Fixes #49100

Currently this PR blocks #49169.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • No

@pullapprove pullapprove bot requested a review from alxhub February 16, 2023 23:29
This was referenced Feb 16, 2023
Closed
Open
@JeanMeche
Copy link
Member Author

I wouldn't expect this commit to trigger regressions. Are the failling internal tests a false positive ?

@dylhunn
Copy link
Contributor

dylhunn commented Feb 28, 2023 •
edited

The presubmit failures are numerous but apparently unrelated. This probably got bundled for presubmit with a bad pending change. Let's run it again.

edit: presubmit running

@dylhunn dylhunn added the action: presubmit The PR is in need of a google3 presubmit label Feb 28, 2023
@jessicajaniuk jessicajaniuk added the area: compiler Issues related to `ngc`, Angular's template compiler label Mar 2, 2023
@ngbot ngbot bot added this to the Backlog milestone Mar 2, 2023
@JeanMeche
Copy link
Member Author

@dylhunn How did the presubmit go ?

@JeanMeche JeanMeche mentioned this pull request May 9, 2023
Closed
@JeanMeche JeanMeche force-pushed the fix/48558 branch 2 times, most recently from 5425ea6 to ecbaee1 Compare June 5, 2023 20:40
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jun 9, 2023
@alan-agius4
Copy link
Contributor

Presubmit

@dylhunn
Copy link
Contributor

dylhunn commented Jun 9, 2023

Just checked Alan's new presubmit. It's clean, modulo 12 flakes and preexisting failures. So we should be good to go in that regard.

JoostK
JoostK reviewed Jun 10, 2023
View reviewed changes
packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved
packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved
@JeanMeche JeanMeche force-pushed the fix/48558 branch 2 times, most recently from 7236b4b to a3cac72 Compare June 10, 2023 20:41
@JoostK JoostK mentioned this pull request Jun 12, 2023
Closed
JoostK
JoostK approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

Overall I think it's time to take a step back and maybe revisit some of the approach here, but this change is in the right direction. Left two minor comments but otherwise LGTM.

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved
packages/compiler/test/shadow_css/host_and_host_context_spec.ts Show resolved Hide resolved
@JoostK JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews risky Identifies a PR that has a level of risk to merging action: global presubmit The PR is in need of a google3 global presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Jun 20, 2023
@JoostK
Copy link
Member

JoostK commented Jun 20, 2023

Labelling as risky since this might affect rendering results in a subtle way. I recommend syncing this separately once TGP comes back green.

@JoostK
Copy link
Member

JoostK commented Jun 21, 2023

@JeanMeche could you also fix the typo in the commit message (attributes is misspelled)

@JeanMeche
fix(compiler): apply style on :host attributes in prod builds.
4ee1138 
In prod builds, selectors are optimized and spaces a removed. angular#48558 introduced a regression on selectors without spaces. This commit fixes tihs.

Fixes angular#49100
@JoostK JoostK 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 Aug 4, 2023
@JeanMeche JeanMeche mentioned this pull request Sep 12, 2023
Closed
@dylhunn dylhunn modified the milestones: Backlog, v17-final Oct 10, 2023
@dylhunn dylhunn self-assigned this Oct 10, 2023
@dylhunn
Copy link
Contributor

dylhunn commented Oct 10, 2023 •
edited

One more presubmit in cl/572379101 -- presubmit is green.
Edit: actually, this needs a global as well. I will run it tonight.

TGP is running: https://test.corp.google.com/OCL:572379101:BASE:572432933:1696991069679:95503e21
Deflake run: https://fusion2.corp.google.com/presubmit/572379101/OCL:572379101:BASE:572432933:1696998499938:6d790136/targets

@dylhunn
Copy link
Contributor

dylhunn commented Oct 11, 2023 •
edited

The global presubmit shows one broken cloud console test. Very frustrating, to be blocked on a single breakage. I have reached out to the code owners to try to get their help fixing it.

@dylhunn
Copy link
Contributor

dylhunn commented Oct 11, 2023 •
edited

The owning team confirms the g3 failure as a flake.

Caretaker: TGP is green, good to merge. I suggest syncing on its own.

@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: global presubmit The PR is in need of a google3 global presubmit labels Oct 11, 2023
@atscott atscott added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release target: major This PR is targeted for the next major release labels Oct 11, 2023
@atscott
Copy link
Contributor

atscott commented Oct 11, 2023

This PR was merged into the repository by commit 0198d21.

atscott pushed a commit that referenced this pull request Oct 11, 2023
@JeanMeche @atscott
fix(compiler): apply style on :host attributes in prod builds. (#49118)
073ebfe 
In prod builds, selectors are optimized and spaces a removed. #48558 introduced a regression on selectors without spaces. This commit fixes tihs.

Fixes #49100

PR Close #49118
@atscott atscott closed this in 0198d21 Oct 11, 2023
@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 Nov 11, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@JeanMeche @ChellappanRajan
fix(compiler): apply style on :host attributes in prod builds. (angul…
137555b 
…ar#49118)

In prod builds, selectors are optimized and spaces a removed. angular#48558 introduced a regression on selectors without spaces. This commit fixes tihs.

Fixes angular#49100

PR Close angular#49118
@JeanMeche JeanMeche deleted the fix/48558 branch February 29, 2024 13:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoostK JoostK JoostK approved these changes

Assignees

@dylhunn dylhunn

Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risky Identifies a PR that has a level of risk to merging target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
v17-final
6 participants
@JeanMeche @dylhunn @alan-agius4 @JoostK @atscott @jessicajaniuk