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): insert APP_ID in styles, contentAttr and hostAttr #17745

Conversation

@petersalomonsen
Copy link
Contributor

commented Jun 24, 2017

PR Checklist

Does please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: 16676

What is the new behavior?

APP_ID is inserted between nghost/ngcontent and component id in styles, contentAttr and hostAttr as it is supposed to do according to the documentation (https://angular.io/guide/component-styles)

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Existing tests have been modified to check for the APP_ID presence. Documentation already states that this should be present, so the change is bringing the implementation closer to how the documentation says it should work.

@splincode

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

@petersalomonsen what is the status of your pull request? is it still relevant? please fix conflicts if they exist

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

@splincode It is still relevant. Without it you will have overlapping styles when having multiple angular apps on the same page, which is caused by the app id no being included as it is supposed to be according to the documentation here: https://angular.io/guide/component-styles#inspecting-generated-css

@glipecki

This comment has been minimized.

Copy link

commented Mar 27, 2018

Additionally, current behavior is misleading as APP_ID documentation states that APP_ID should be used as a part of generated attributes for ViewEncapsulation.Emulated (https://angular.io/api/core/APP_ID).

This PR would help me a lot! :-)

@glipecki

This comment has been minimized.

Copy link

commented Mar 27, 2018

@petersalomonsen Are you going to fix PR checks?

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@glipecki Can have a look at it.

petersalomonsen added a commit to petersalomonsen/angular that referenced this pull request Mar 27, 2018

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

packages/core/test/render3/imported_renderer2.ts was added after my initial PR. The DomRenderer2Factory was instanced without the app id parameter (that I added in my PR), so I added a dummy app id to get around it. Tests passed locally so hopefully passes the CI too? (I'm not 100% up to speed on all that's going on in the CI tests).

@splincode

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@petersalomonsen This pull request is big!

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@splincode Did I merge / rebase the wrong way?

I did this:

git rebase master -i
git push -f

(as written here: https://github.com/angular/angular/blob/master/CONTRIBUTING.md - if changes are suggested (10))

I've only made two commits. These are the only changes: master...petersalomonsen:16676-viewencapsulation-appid

@petersalomonsen petersalomonsen force-pushed the petersalomonsen:16676-viewencapsulation-appid branch from eef0bb0 to c17cf8c Mar 27, 2018

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@splincode Should never have used the sync button in visual studio code that created a merge commit. Merge commit is removed now.

@splincode

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@petersalomonsen Can you edit the code according to the formatting rules (default tslint.json)?

@splincode

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@petersalomonsen Use interactive git rebase to combine your little commits into one

@petersalomonsen petersalomonsen force-pushed the petersalomonsen:16676-viewencapsulation-appid branch from c17cf8c to 623edf2 Mar 27, 2018

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@splincode Now combined into one commit. And I've also run gulp format.

@petersalomonsen petersalomonsen force-pushed the petersalomonsen:16676-viewencapsulation-appid branch 3 times, most recently from 65f79bc to f73b907 Mar 27, 2018

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

All CI tests are passing now. What more can I do for this PR to be accepted?

@aaaa0441

This comment has been minimized.

Copy link

commented Apr 30, 2018

@petersalomonsen this PR looks great! I also need this fix to allow our micro-apps to have non-conflicting styles when using https://github.com/CanopyTax/single-spa.

Do you know what's preventing this PR from being approved/merged? (Sorry that I'm not familiar with the contribution process.)

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

@aaaa0441 I think the remaining depends on the Angular repository managers now, but if there's something more I can do to complete the merge I'd be happy to help - so let me know.

@aaaa0441

This comment has been minimized.

Copy link

commented Apr 30, 2018

@petersalomonsen Thanks for responding. I got your changes and applied the diff to my local node_modules/@angular/platform-browser dependency using patch-package. Works well.

I definitely prefer Angular to have this fixed instead of monkey patching it the way I am doing. However, I have no other way for our project to move on now. =(

Really hope this gets merged soon. Who can we @ to get this expedited?

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2018

@aaaa0441 I think @chuckjaz is member of the Angular team and maybe can give some info on what needs to be done to get the merge through.

@jasonaden jasonaden added this to the needsTriage milestone Jan 29, 2019

@mleibman

This comment has been minimized.

Copy link

commented Feb 7, 2019

An easier fix might be to use a global (window-scope) variable for the auto-incremented component ID instead of a module-level one:

let _renderCompCount = 0;
export function resolveRendererType2(type?: RendererType2 | null): RendererType2|null {
if (type && type.id === UNDEFINED_RENDERER_TYPE_ID) {
// first time we see this RendererType2. Initialize it...
const isFilled =
((type.encapsulation != null && type.encapsulation !== ViewEncapsulation.None) ||
type.styles.length || Object.keys(type.data).length);
if (isFilled) {
type.id = `c${_renderCompCount++}`;
} else {
type.id = EMPTY_RENDERER_TYPE_ID;
}
}
if (type && type.id === EMPTY_RENDERER_TYPE_ID) {
type = null;
}
return type || null;
}

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Problem with a window-scoped auto-increment component id is that it would not be according to the documentation: https://angular.io/guide/component-styles#inspecting-generated-css - where you can see that the app id is part of the component id.

As said in the beginning this pull request is just about making it work as the documentation says it should.

@bogdan-kuternoga

This comment has been minimized.

Copy link

commented Apr 2, 2019

@petersalomonsen I think you have to make some party when this PR will be merged xD

@splincode @mhevery @kara
But if seriously, such approach to code review discourages.

I don't want to tell you how to prioritize issues, I just can tell how it looks from user perspective :) . And as for me something what is documented and doesn't work in 100% cases should be fixed ASAP instead of waiting for 2 years...

@mhevery

mhevery approved these changes Apr 8, 2019

@ngbot

This comment has been minimized.

Copy link

commented Apr 8, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: test_ivy_aot" is failing
    failure status "code-review/pullapprove" is failing
    pending missing required labels: PR target: *
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mhevery mhevery force-pushed the petersalomonsen:16676-viewencapsulation-appid branch from 530486e to db0818c Apr 8, 2019

@petersalomonsen petersalomonsen requested review from IgorMinar and angular/fw-core as code owners Apr 8, 2019

@mhevery

This comment has been minimized.

@mhevery mhevery self-assigned this Apr 8, 2019

@mleibman

This comment has been minimized.

Copy link

commented Apr 9, 2019

@petersalomonsen looks like this is failing some unit tests, including a regex check for the _nghost attribute in

/<host-cmp>foo<\/host-cmp><embedded-cmp _nghost-c(\d+)="">From module injector<\/embedded-cmp>/);

@petersalomonsen petersalomonsen force-pushed the petersalomonsen:16676-viewencapsulation-appid branch 5 times, most recently from ce1642d to 633825f Apr 9, 2019

@petersalomonsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@mleibman I believe the regex checks are fixed now. The circle_ci integration test passed on the previous push, and there were no other changes than a few spaces to please the linter (sorry for not sorting this out locally before pushing).

@mleibman

This comment has been minimized.

Copy link

commented Apr 9, 2019

@petersalomonsen Thanks! I'm not an Angular team member, so I can't review/merge it myself. I'm just facilitating since I depend on this fix :)

@mhevery mhevery force-pushed the petersalomonsen:16676-viewencapsulation-appid branch from 633825f to 9e3d82f Apr 9, 2019

@mhevery

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

CARETAKER: The build failure is bazel build time, which is unrelated to this PR, please merge manually.

@IgorMinar IgorMinar force-pushed the petersalomonsen:16676-viewencapsulation-appid branch from 9e3d82f to 56dc55d Apr 11, 2019

IgorMinar added a commit that referenced this pull request Apr 11, 2019

@IgorMinar IgorMinar closed this in 712d60e Apr 11, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.