fix(core): prevent stash listener conflicts#59635
fix(core): prevent stash listener conflicts#59635arturovt wants to merge 1 commit intoangular:mainfrom
Conversation
93414f0 to
20b19ab
Compare
7c91668 to
f29a5a8
Compare
thePunderWoman
left a comment
There was a problem hiding this comment.
We'll definitely need some tests here for this so we don't regress in the future. Can you add those?
b10f3c2 to
021363a
Compare
The stash event listener is a global function that might be unsafely overridden if multiple microfrontend applications exist on the page. In this commit, we create a map of `APP_ID` to stash event listener functions. This map prevents conflicts because multiple applications might be bootstrapped simultaneously on the client (one rendered on the server and one rendering only on the client). I.e., the code that might be used is: ```ts // Given that `app-root` is rendered on the server bootstrapApplication(AppComponent, appConfig); bootstrapApplication(BlogRootComponent, appBlogConfig); ``` Two bootstrapped applications would conflict and override each other's code.
021363a to
ec56161
Compare
|
@thePunderWoman added unit test. |
thePunderWoman
left a comment
There was a problem hiding this comment.
LGTM! Thanks for adding the unit test.
|
@arturovt I'm switching this to target the next major release since it has merge conflicts with the patch branch. If you want the change in a patch release you'll need to create a separate PR specifically to the 19.2.x branch. |
Patch version of angular#59635.
|
This PR was merged into the repository by commit 624be2e. The changes were merged into the following branches: main |
| const appId = lView[INJECTOR].get(APP_ID); | ||
| const stashEventListener = stashEventListeners.get(appId); | ||
| stashEventListener?.(target as RElement, eventName, wrappedListener); |
There was a problem hiding this comment.
@arturovt thanks for the improvement 👍
I think we should optimize this code path by moving the logic into the stashEventListener function (or a function that manages its assignment). Otherwise, we invoke this code for all listeners all the time, when we only need it in case the event replay feature is enabled (explicitly or via incremental hydration).
There was a problem hiding this comment.
I get your comment. I'm wondering whether we should be checking if event replay feature is enabled in this code path and only then stash the event listener. I have no ideas on having that functionality inside the stashEventListener, I mean I don't get how it would technically look like.
If I understand correctly, stashing is called every time when listener is added, basically like this:
Element.prototype.addEventListener = function (
this: Element,
type: string,
listener: EventListenerOrEventListenerObject,
options?: boolean | AddEventListenerOptions,
) {
const stash = stashEventListeners.get(appId);
stash?.(this, type, listener as VoidFunction);
return originalAddEventListener.call(this, type, listener, options);
};Another question is, should we clear the stash event listener once the hydration is done to avoid stashing it after it's functioning in the browser?
There was a problem hiding this comment.
let stashEventListenerImpl = (
lView: LView,
target: RElement,
eventName: string,
wrappedListener: EventCallback,
) => {};
export function enableStashEventListenerImpl() {
stashEventListenerImpl = (
lView: LView,
target: RElement,
eventName: string,
wrappedListener: EventCallback,
) => {
const appId = lView[INJECTOR].get(APP_ID);
const stashEventListener = stashEventListeners.get(appId);
stashEventListener?.(target as RElement, eventName, wrappedListener);
};
}And in the code:
stashEventListenerImpl(lView, target, eventName, wrappedListener);There was a problem hiding this comment.
That would avoid an injector lookup and a Map lookup every time the listener() is called unless the replay is enabled.
There was a problem hiding this comment.
Yes, the solution described in #59635 (comment) looks great and something that I had in mind as well, thank you 👍
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The stash event listener is a global function that might be unsafely overridden if multiple microfrontend applications exist on the page.
In this commit, we create a map of
APP_IDto stash event listener functions. This map prevents conflicts because multiple applications might be bootstrapped simultaneously on the client (one rendered on the server and one rendering only on the client).I.e., the code that might be used is:
Two bootstrapped applications would conflict and override each other's code.