-
Notifications
You must be signed in to change notification settings - Fork 26.2k
fix(core): prevent stash listener conflicts #59635
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.< 8000 /p>
Already on GitHub? Sign in to your account
Conversation
93414f0 to
20b19ab
Compare
7c91668 to
f29a5a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll definitely need some tests here for this so we don't regress in the future. Can you add those?
b10f3c2 to
021363a 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the solution described in #59635 (comment) looks great and something that I had in mind as well, thank you 👍
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.