[lexical] Bug Fix: allow same mutation listener fn to be registered to multiple nodes#7654
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
etrepum
left a comment
There was a problem hiding this comment.
Seems like the right thing to do, honestly surprised it worked this way. I guess most use cases were defining this function with a closure or just not registering multiple classes.
| ).klass; | ||
| const mutations = this._listeners.mutation; | ||
| mutations.set(listener, klassToMutate); | ||
| if (!mutations.has(listener)) { |
There was a problem hiding this comment.
Just a small nit but if the same sort of klassSet variable approach is used here there could be a single get instead of has and then get. We don't need to check separately because undefined is not a valid value for the entry to have.
There was a problem hiding this comment.
Something like this?
let klassSet = mutations.get(listener);
if (klassSet === undefined) {
klassSet = new Set();
mutations.set(listener, klassSet);
}
klassSet.add(klassToMutate);
I'm happy either way. I went with the current approach to avoid let.
There was a problem hiding this comment.
Yeah, I think that using let is a lesser sin than having more runtime code (and a non-null type assertion). There's plenty of mutable variables in this codebase
Description
registerMutationListenercurrently maps from the listener function to the corresponding node class. If you then callregisterMutationListenerwith a different class, it overwrites the first. This also means that the cleanup function from the first class could end up removing the listener for the second class.registerNodeTransformon the other hand already allows you to register the same listener function to multiple classes. This change brings the same behaviour toregisterMutationListener.Test plan
Added unit test, fails before change, passes after.