@thePunderWoman - Hey 👋

This is the new PR as we discussed in #62357 where i've taken the approach of migrating anyone on the existing shadowdom onto a new LegacyShadowDom and marked that as deprecated, so that we don't break anything for existing users.

I have manually tested running the migration locally, seems to work! One thing i am not 100% on though, as I predominantly use NX (which overrides ng commands) and runs it's own migrations on upgrades - is does Angular automatically run these migrations when you do an ng upgrade ? If so, that's good (kind of what we'd want i think)

Thanks!

As is, this is considered a breaking change as the ShadowDom encapsulation changes behavior (even with the migration).

This probably needs to be discussed if the best is to keep ShadowDom as is (would reduce the burden of landing the change) and introduce a new symbol for the new behavior.

As is, this is considered a breaking change as the ShadowDom encapsulation changes behavior (even with the migration).

This probably needs to be discussed if the best is to keep ShadowDom as is (would reduce the burden of landing the change) and introduce a new symbol for the new behavior.

I'll mark as breaking 👍

Yeah the other option i mentioned was just creating a new encapsulation mode with the "fixed" shadowdom

But I leaned towards this as any new apps/components spun up would get the fixed behaviour, and it was suggested in the other PR it would be a lot of work for the team/users to change everything if you landed just the fixed version. By marking it as deprecated users would at least be nudged in the right direction, and hopefully make the Angular teams job a bit easier in the future 🤞

Otherwise we're just kicking the can down the road and you'll have two sets of encapsulations for shadowdom (arguably confusing), more to maintain, and in theory more components to fix later down the line when you pick it up from your backlog

Regarding priority of this PR: I was recently exploring using shadow DOM to isolate styles across multiple applications within the same page (MicA). One challenge is that CdkOverlay renders outside the main application root, so any styles are lost there. I had explored creating a shadow root there as well and noticed the same problem that styles for the entire application are unnecessarily duplicated in the extra root. I think this PR might be useful for that effort, but I'm not yet sure if using shadow roots is the right approach for the broader style isolation problem I'm looking at in that context, hopefully I can get more insight on that soon.

Regarding this PR more specifically: I wonder if this setting should be configured via dependency injection rather than creating a new ViewEncapsulation mode? It feels easier to configure a single token rather than migrating each component individually.

A token does potentially make it a bit harder to incrementally roll out this change, but I suspect it's not that breaking and maybe doesn't need to have a super fine-grained opt-out? You could certainly use DI to opt-out specific components, though the hierarchy is kind of working against you there since opting-out one component naturally opts-out its dependencies, which isn't really what you want.

I'm mainly just asking whether this is worth a new ViewEncapsulation mode or whether we can do something simpler with DI? I'll defer to other framework folks on whether that's worth seriously considering, just an idea.

@ryan-bendel Looks like this needs a few goldens updated. yarn run symbol-extractor:update should update the bundle symbols and yarn bazel run //packages/core:core_api.accept should update the core symbols. Can you do that and then re-push? That should get CI closer to green.

@thePunderWoman ohhh I didn't know about that step, thanks! Pushed that now

I'll look at the failing tests, will run the same command locally the action is using.

Not 100% clear on what is needed in some targets, for example, pkg_npm only has one migration (and my new one) defined, but the migrations folder has multiple migrations in it. So likely i'm misunderstanding what is supposed to be defined where.

Side note, i've never once been able to run the test_firefox tests without it falling over 😆

Grrr. I might need a bit of help with this if one of the Angular team wouldn't mind jumping on my branch?

ERROR: /home/runner/work/angular/angular/packages/core/schematics/migrations/shadowdom-migration/BUILD.bazel:10:11: //packages/core/schematics/migrations/shadowdom-migration:shadowdom-migration_rjs: no such attribute 'interop_deps' in 'ts_project' rule

I can't work out what is going on here, this build file is identical in all but name to the test-bed-get migration, but fails.

Really struggling to run the full test suite locally because it can't seem to locate the Firefox bin and bombs out. Just doing a build of the core package passes, so i've no clue what's going on 😢

@ryan-bendel Looks like there were some recent change with these bazel rules. interop_deps can be rolled into deps. I updated it and pushed, and I also collapsed the commits into the one. Hopefully it should not have issues with that during build anymore.

@ryan-bendel Looks like there were some recent change with these bazel rules. interop_deps can be rolled into deps. I updated it and pushed, and I also collapsed the commits into the one. Hopefully it should not have issues with that during build anymore.

@thePunderWoman you're a star ⭐ - I think i tried that in a previous commit, and may have ended up failing elsewhere so i reverted, lets see how it goes this time 🤞

Ah yes:

packages/core/schematics/migrations/shadowdom-migration/index.ts:10:36 - error TS2307: Cannot find module '../../utils/tsurge/helpers/angular_devkit' or its corresponding type declarations.

10 import {runMigrationInDevkit} from '../../utils/tsurge/helpers/angular_devkit';

Which confused me, as it's the same import path as other migrations. I'd lost the will to live at this point 😆

@ryan-bendel looks like it did get past that problem, but now has ✨other✨ problems. I can take a look.

@ryan-bendel looks like it did get past that problem, but now has ✨other✨ problems. I can take a look.

The index.ts one is odd, as comparing that index to the inject-flags index, the import paths are the same 🤔

Similar confusion with the overrides, i.e override async analyze as they appear to match what the other migrations do 🤷

Thank you for your help! Much appreciated

@ryan-bendel Bazel can be confusing. I figured out what needed to be in the deps for it to build.

Pushed the test fixes and changes requested, will squash commits when the build is green

@ryan-bendel you have a tiny docs conflict. Once you resolve that, this should be merge ready!!!

@thePunderWoman merge resolved 😃 - thanks everyone for this

This PR was merged into the repository by commit d24d574.

The changes were merged into the following branches: main

oh this is a great feature that we desperately need