Skip to content
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.

Already on GitHub? Sign in to your account

Add Shiny Event classes for custom events #3815

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

gadenbuie
Copy link
Member

This is an internal change that should not change user-facing behavior, but does improve the developer experience when writing Typescript that builds on Shiny's custom events, e.g. shiny:value or shiny:inputchanged.

The immediate need was encountered in bslib, where we'd like to write an event handler for shiny:value, e.g.

$(document).on("shiny:value", function(event: ShinyEventValue) {
  // use a prop of event that is specific to Shiny's event
  const binding = event.binding
})

Previously, we exported the ShinyEventValue interface, but didn't extend jQuery's on() function signature to associate that interface with the "shiny:value" event type. We now register these custom event names with the custom event interfaces so that type information is available to developers using these events.

Finding these event names revealed that, while the interfaces were defined in srcts/src/events/shinyEvents.ts, we were creating the events elsewhere with e.g. $.Event("shiny:value"), sometimes in more than one place. This PR adds to the event interface definitions with a new set of classes, named e.g. EventValue and EventInputChanged, that are now used to create Shiny's events. This consolidates the event implementation in one place.

Beyond consolidation and improved consistency, this PR also creates an EventBase class that serves as an adapter to JQuery.Event. Because all uses of the Event* classes flow through this base class, this gives us flexibility in the future to change the underlying event mechanism (e.g. to use the standard browser Event API).

Also mark `priority` as optional since it was optionally provided in practice
These classes create the custom Shiny events and help us extend JQuery's event handlers for these event types.
We now do this in place rather than as a separate function,
i.e. we replace `triggerFileInputChanged()` with our improved
abstraction.
…her than `evt`

It turns out the props on EventCommon aren't used everywhere
It was already used by preset-typescript, but we need to use the
plugin so that the transpilation happens at the right time, in
particular because we are using `declare` within classes.
* Use getters to expose data from `this.event` as if they were top-level
* Refer to the interfaces directly rather than repeating type definitions
Copy link
Contributor

@nstrayer nstrayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. I'm curious about the use of any, though. It's entirely possible I'm missing something in the logic that explains why we need any over unknown. If it's due to unknown being too restrictive, we can add assertion/predicate functions where necessary to make sure the data actually looks correct as it passes through.

@@ -54,3 +54,5 @@ declare global {
): this;
}
}

export type { EvtFn };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this stands for "Event Function"? If so Is there a reason to keep it so abbreviated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mostly because it was reused often in this file and because evt is used as the convention for "jQuery event" elsewhere in the codebase.

import type { EvtFn } from "./jQueryEvents";
import $ from "jquery";

// This class implements a common interface for all Shiny events, and provides a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This class implements a common interface for all Shiny events, and provides a
/**
* This class implements a common interface for all Shiny events and provides a
* layer of abstraction between the Shiny event and the underlying jQuery event
* object. We use a new class, rather than extending JQuery.Event, because
* jQuery.Event is an old function-style class. Each Event class has a
* corresponding ShinyEvent interface that describes the event object that is
* emitted. At the end of this file, we extend JQuery's `on()` method to
* associate the ShinyEvent interfaces with their corresponding event string.
*/
class EventBase {
event: JQuery.Event;
/**
* Constructor for the EventBase class.
*
* @param {string} type The event type.
*/
constructor(type: string) {
this.event = $.Event(type);
}
/**
* Triggers the event on the specified element or the document.
*
* @param {HTMLElement | JQuery<HTMLElement> | typeof document | null} el The element to trigger the event on, or `null` for the document.
*/
triggerOn(
el: HTMLElement | JQuery<HTMLElement> | typeof document | null
): void {
$(el || window.document).trigger(this.event);
}
/**
* Checks if the default action of the event has been prevented.
*
* @returns {boolean} `true` if the default action has been prevented, `false` otherwise.
*/
isDefaultPrevented(): boolean {
return this.event.isDefaultPrevented();
}
}

Using JSDoc syntax so docs are given on mouseover using intellisense

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSDoc is nice; it's a shame it doesn't pick up type definitions though. I'll look into adding JSDoc comments in the code I'm editing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10c2398


interface ShinyEventCommon extends JQuery.Event {
name: string;
value: unknown;
el: HTMLElement | null;
value: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why any here? Typically unknown is safer.

Copy link
Collaborator

@schloerke schloerke May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl/dr; My current understanding is to use any as we can not change the type of unknown to another value in a sub class.

It comes from https://en.wikipedia.org/wiki/SOLID's The Liskov substitution principle:

Functions that use pointers or references to base classes must be able to use objects of derived classes without knowing it.

I broke this rule all over the place in the original conversion within rstudio/shiny. (This rule alone warrants a rewrite of most of the unknown types to any or Generic types.)

For example: Let say ShinyEventCommon has value: unknown. Let's have NickEventInputChanged extend ShinyEventCommon. Even if you know NickEventInputChanged's value has type NickValue, the value must satisfy it's parent's class value type of unknown. This forces NickEventInputChanged to then have a value: unknown. If ShinyEventCommon had value: any, then NickEventInputChanged can have value: customType as any is more generic than customType.


But after writing this all out, I am thinking of any vs never. The never type can not be recovered. The unknown type requires some type checking but can be eventually be used (it just requires that you type check it).

I think it'd be good to have a quick discussion at the end of standup today.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this all go away if we used a generic type on value where T extends any and value: T. Then we don't need to worry what the type as it is always defined by the Generic type.

interface ShinyEventUpdateInput extends ShinyEventCommon {
message: unknown;
message?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here about any.

@@ -227,14 +227,14 @@ class FileUploader extends FileProcessor {
// Trigger shiny:inputchanged. Unlike a normal shiny:inputchanged event,
// it's not possible to modify the information before the values get
// sent to the server.
const evt = triggerFileInputChanged(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much nicer

@@ -27,6 +27,7 @@ rules:
- off
"@typescript-eslint/explicit-module-boundary-types":
- error
- allowArgumentsExplicitlyTypedAsAny: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments below on this. I'd imagine we can unset this again. Might require some assertions but that's probably safer anyways.

@gadenbuie gadenbuie self-assigned this May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants