Scoped external callbacks #57815
Replies: 5 comments 4 replies
-
Thanks @luisherranz! I'm obviously biased, but to me the benefits of proposal 1. is that it avoids having the developer to think about initializing those listeners. IMO if overall scrolling should trigger a change on an element, it's most intuitive to have the directive on that element, and all other proposals still require "manually" adding an event listener, which is more complicated. To me it feels particularly out of place in the Interactivity API since event listeners are otherwise added through directives, without having to write the relevant +1 to exposing the |
Beta Was this translation helpful? Give feedback.
-
I'll premise with saying that my hands-on experience with the Interactivity API is still limited, so I haven't encountered this situation myself as of yet. After reading @luisherranz 's post I was mostly convinced by the My only concern at first, was that sometimes functions like these can create very complex scenarios with TypeScript (I'm not sure how much you are thinking about them). With that said, after reading @felixarntz's response, I'm more convinced that the recommended use-case should be using the directives (which I wasn't particularly a fan of at first). But now I think actually this approach not only enhances developer experience but also maintains consistency within the API design, which is very important in my opinion: internal consistency makes learning quite easier. So, +1 for this solution as it directly addresses the common use case and does so in a clean, intuitive manner, and for keeping the As for the other proposals, I think they are overall interesting but in general less intuitive and feel a bit more finicky/arbitrary. Maybe in the future we could also explore some sort of context-aware event listener registration within the API, which automatically handles scope management without needing additional directives or utilities, but I'm not sure what shape it could take and whether it would have any advantage over the proposals here… rather, it would mostly likely just hide intention and make things more convoluted. |
Beta Was this translation helpful? Give feedback.
-
I don't know what I was thinking, this is totally fine as well. The reference that is passed to the event listener is extracted when there's still scope: const { state, callbacks } = store("myPlugin", {
callbacks: {
onScroll() {
// ...
},
initGlobalListener() {
window.addEventListener("scroll", callbacks.onScroll);
},
},
}); I've corrected the opening post. |
Beta Was this translation helpful? Give feedback.
-
This is a bit long, but as an exercise and to see a more real-life scenario, I'm going to try the different approaches with this piece of Woo's code, which is already using external callbacks this way, and even an external callback ( This is the current code. It's using technique number 3: Use the callback closure to get references to the scope It needs to get a reference to the const selectImage = (
context: ProductGalleryContext,
select: "next" | "previous"
) => {
const imagesIds =
context[
context.isDialogOpen ? "dialogVisibleImagesIds" : "visibleImagesIds"
];
const selectedImageIdIndex = imagesIds.indexOf(context.selectedImage);
const nextImageIndex =
select === "next"
? Math.min(selectedImageIdIndex + 1, imagesIds.length - 1)
: Math.max(selectedImageIdIndex - 1, 0);
context.selectedImage = imagesIds[nextImageIndex];
};
const closeDialog = (context: ProductGalleryContext) => {
context.isDialogOpen = false;
context.selectedImage = context.firstMainImageId;
};
store("woocommerce/product-gallery", {
actions: {
closeDialog: () => {
const context = getContext();
closeDialog(context); // bad
},
selectNextImage: (event: MouseEvent) => {
event.stopPropagation();
const context = getContext();
selectImage(context, "next"); // bad
},
selectPreviousImage: (event: MouseEvent) => {
event.stopPropagation();
const context = getContext();
selectImage(context, "previous"); // bad
},
},
callbacks: {
keyboardAccess: () => {
const context = getContext();
let allowNavigation = true;
const handleKeyEvents = (event: KeyboardEvent) => {
if (!allowNavigation || !context.isDialogOpen) {
return;
}
// Disable navigation for a brief period to prevent spamming.
allowNavigation = false;
requestAnimationFrame(() => {
allowNavigation = true;
});
// Check if the esc key is pressed.
if (event.code === "Escape") {
closeDialog(context); // bad
}
// Check if left arrow key is pressed.
if (event.code === "ArrowLeft") {
selectImage(context, "previous"); // bad
}
// Check if right arrow key is pressed.
if (event.code === "ArrowRight") {
selectImage(context, "next"); // bad
}
};
document.addEventListener("keydown", handleKeyEvents);
return () => document.removeEventListener("keydown", handleKeyEvents);
},
},
}); This would be the code using technique 2: Expose a const { actions } = store("woocommerce/product-gallery", {
actions: {
closeDialog: () => {
const context = getContext();
context.isDialogOpen = false;
context.selectedImage = context.firstMainImageId;
},
selectImage: (select: "next" | "previous" = "next") => {
const context = getContext();
const imagesIds =
context[
context.isDialogOpen ? "dialogVisibleImagesIds" : "visibleImagesIds"
];
const selectedImageIdIndex = imagesIds.indexOf(context.selectedImage);
const nextImageIndex =
select === "next"
? Math.min(selectedImageIdIndex + 1, imagesIds.length - 1)
: Math.max(selectedImageIdIndex - 1, 0);
context.selectedImage = imagesIds[nextImageIndex];
},
selectNextImage: (event: MouseEvent) => {
event.stopPropagation();
actions.selectImage("next");
},
selectPreviousImage: (event: MouseEvent) => {
event.stopPropagation();
actions.selectImage("previous");
},
},
callbacks: {
keyboardAccess: () => {
let allowNavigation = true;
const handleKeyEvents = withScope((event: KeyboardEvent) => {
const context = getContext();
if (!allowNavigation || !context.isDialogOpen) {
return;
}
// Disable navigation for a brief period to prevent spamming.
allowNavigation = false;
requestAnimationFrame(() => {
allowNavigation = true;
});
// Check if the esc key is pressed.
if (event.code === "Escape") {
actions.closeDialog();
}
// Check if left arrow key is pressed.
if (event.code === "ArrowLeft") {
actions.selectPreviousImage();
}
// Check if right arrow key is pressed.
if (event.code === "ArrowRight") {
actions.selectNextImage();
}
});
document.addEventListener("keydown", handleKeyEvents);
return () => document.removeEventListener("keydown", handleKeyEvents);
},
},
}); This is better. At least we can use the actions as they are, without having to refactor everything to pass Finally, with technique 1: Add a global event listener directive let allowNavigation = true;
const { actions } = store("woocommerce/product-gallery", {
actions: {
closeDialog: () => {
const context = getContext();
context.isDialogOpen = false;
context.selectedImage = context.firstMainImageId;
},
selectImage: (select: "next" | "previous" = "next") => {
const context = getContext();
const imagesIds =
context[
context.isDialogOpen ? "dialogVisibleImagesIds" : "visibleImagesIds"
];
const selectedImageIdIndex = imagesIds.indexOf(context.selectedImage);
const nextImageIndex =
select === "next"
? Math.min(selectedImageIdIndex + 1, imagesIds.length - 1)
: Math.max(selectedImageIdIndex - 1, 0);
context.selectedImage = imagesIds[nextImageIndex];
},
selectNextImage: (event: MouseEvent) => {
event.stopPropagation();
actions.selectImage("next");
},
selectPreviousImage: (event: MouseEvent) => {
event.stopPropagation();
actions.selectImage("previous");
},
},
callbacks: {
keyboardAccess: () => {
const context = getContext();
if (!allowNavigation || !context.isDialogOpen) {
return;
}
// Disable navigation for a brief period to prevent spamming.
allowNavigation = false;
requestAnimationFrame(() => {
allowNavigation = true;
});
// Check if the esc key is pressed.
if (event.code === "Escape") {
actions.closeDialog();
}
// Check if left arrow key is pressed.
if (event.code === "ArrowLeft") {
actions.selectPreviousImage();
}
// Check if right arrow key is pressed.
if (event.code === "ArrowRight") {
actions.selectNextImage();
}
},
},
}); Even better. But now, if instead of a regular variable const { actions } = store("woocommerce/product-gallery", {
actions: {
// ...
disableNavigation() {
const ctx = getContext();
ctx.allowNavigation = false;
},
enableNavigation() {
const ctx = getContext();
ctx.allowNavigation = true;
},
},
callbacks: {
keyboardAccess: () => {
const { allowNavigation, isDialogOpen } = getContext();
if (!allowNavigation || !isDialogOpen) {
return;
}
// Disable navigation for a brief period to prevent spamming.
actions.disableNavigation();
requestAnimationFrame(
withScope(() => {
actions.enableNavigation(); // Without `withScope`, this would fail.
})
);
// Check if the esc key is pressed.
if (event.code === "Escape") {
actions.closeDialog();
}
// Check if left arrow key is pressed.
if (event.code === "ArrowLeft") {
actions.selectPreviousImage();
}
// Check if right arrow key is pressed.
if (event.code === "ArrowRight") {
actions.selectNextImage();
}
},
},
}); Funny thing is that this is very close to be working, but it doesn't. requestAnimationFrame(() => {
actions.enableNavigation(); // There's no scope when we get the reference.
}); This works fine because it's technique 4: Use actions/callbacks to get automatically scoped functions requestAnimationFrame(actions.enableNavigation); // There's scope when we get the reference. But again, it's not totally obvious why, so it'd be confusing to propose it as the only API. With technique 4 only, it would be: let allowNavigation = true;
const { actions } = store("woocommerce/product-gallery", {
actions: {
closeDialog: () => {
const context = getContext();
context.isDialogOpen = false;
context.selectedImage = context.firstMainImageId;
},
selectImage: (select: "next" | "previous" = "next") => {
const context = getContext();
const imagesIds =
context[
context.isDialogOpen ? "dialogVisibleImagesIds" : "visibleImagesIds"
];
const selectedImageIdIndex = imagesIds.indexOf(context.selectedImage);
const nextImageIndex =
select === "next"
? Math.min(selectedImageIdIndex + 1, imagesIds.length - 1)
: Math.max(selectedImageIdIndex - 1, 0);
context.selectedImage = imagesIds[nextImageIndex];
},
selectNextImage: (event: MouseEvent) => {
event.stopPropagation();
actions.selectImage("next");
},
selectPreviousImage: (event: MouseEvent) => {
event.stopPropagation();
actions.selectImage("previous");
},
},
callbacks: {
handleKeyEvents: (event: KeyboardEvent) => {
const context = getContext();
if (!allowNavigation || !context.isDialogOpen) {
return;
}
// Disable navigation for a brief period to prevent spamming.
allowNavigation = false;
requestAnimationFrame(() => {
allowNavigation = true;
});
// Check if the esc key is pressed.
if (event.code === "Escape") {
actions.closeDialog();
}
// Check if left arrow key is pressed.
if (event.code === "ArrowLeft") {
actions.selectPreviousImage();
}
// Check if right arrow key is pressed.
if (event.code === "ArrowRight") {
actions.selectNextImage();
}
},
keyboardAccess: () => {
document.addEventListener("keydown", actions.handleKeyEvents);
return () =>
document.removeEventListener("keydown", actions.handleKeyEvents);
},
},
}); It doesn't solve the requestAnimationFrame(
withScope(() => {
actions.enableNavigation();
})
); requestAnimationFrame(actions.enableNavigation); So all in all and even though technique 4 works in all scenarios where |
Beta Was this translation helpful? Give feedback.
-
Both
Thanks all for your feedback and suggestions 🎉 Closing now as completed, but feel free to keep commenting or reopen. |
Beta Was this translation helpful? Give feedback.
-
In the Interactivity API, the actions, derived state and other callbacks that are referenced in the directives are scoped to the element that triggers them.
For example, in this HTML:
Each time the action (
actions.inc
) is executed, it is scoped to the element that triggered it. That means that if you click on the.inc-1
button, it will modify the first context, and if you click on the.inc-2
button, it will modify the second context.This behavior is natural and intuitive and doesn't need explanation.
The scope is also inherited in other actions/derived state/callbacks that are used in the execution of the initial action/derived state/callback, like this:
In this case,
actions.inc
calls the getter ofstate.isStillSmall
which is accessing the context. When the getter runsgetContext()
, it will inherit the context ofactions.inc
. All this is still natural and intuitive.To make this scope inheritance automatic and avoid the "passing context/store from one function to the next hell" of the old
store()
API, we internally keep the current scope until the execution of the initial action/derived state/callback has finished. This works well, but has one limitation: asynchronicity.That's the reason we switched from async functions to generators: to control the function yields and restore the scope when it resumes:
All this plays out very nicely within the boundaries of the Interactivity API.
Now, there is still a small caveat where actions/derived state/callbacks can lose the scope: external callbacks. Like for example, those in global listeners:
Let's talk about ways to solve this.
Add a global event listener directive
@felixarntz mentioned the possibility of creating a new directive to add listeners for global events, like
data-wp-on-global--{event}
ordata-wp-on-window--{event}
.In this case, the reference is the callback, so we can easily scope it. It will also simplify the developer experience when dealing with global events because it is less verbose than using
data-wp-init
withwindow.addEventListener
.The previous example would become:
I like this solution a lot, and even though this only solves this matter for these types of external callbacks, I think we should do it.
Expose a
withScope
utility@DAreRodz created a
withScope
function that can wrap a callback to save the current scope and restore it when the callback runs.The previous example would become:
I also like this API because it is simple and can be used to restore the scope in any situation.
Use the callback closure to get references to the scope
Another solution would be to educate people to get references to the scope outside of the callback and then use them inside it.
The previous example becomes:
However, you really need to know what you are doing because sometimes the scope might now be so obvious:
So even though it's possible, it's not straightforward and safe. I don't like this solution, but I wanted to document it for completeness.
Use actions/callbacks to get automatically scoped functions
Actions/callbacks are internally wrapped with something similar to
withScope()
. We could also take advantage of that as a way to get a scoped callback without having to use a wrapper.The previous example becomes:
This works because when we do
const onScroll = callbacks.onScroll;
, we get a reference to the scopedonScroll
function, including the wrapper.However, even though this works well, it's a bit convoluted and not totally intuitive: why do you need to create a separate callback?
It's also easy to break. For example, this doesn't work, and it's not immediately obvious why it doesn't:EDIT: My bad, this is totally fine because the reference passed to the function is extracted when there is still a scope.
This is a bit better than getting the context on the closure, but probably more confusing than using
withScope
due to the lack of explicitness. Again, I wanted to document it in case it inspires better ideas.As of today, I'd go with both Felix and David's proposals (1 and 2). The first one provides a better experience and solves the scope problem at the same time for the most common use case, and the second one covers the rest of the external callback cases.
Thoughts? Feedback? Any other ideas? 🙂
Beta Was this translation helpful? Give feedback.
All reactions