-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add Shiny Event classes for custom events #3815
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
base: main
Are you sure you want to change the base?
Changes from 20 commits
01fd8fa
90c0523
8d12ae7
0e2d040
e2d19b9
60f074d
54fa857
63427a7
651b768
922d16d
2740be1
b5c6561
a66ef6d
08ced6b
4f50796
ce4961f
0adeb4b
4648423
eddb0d9
064da3a
10c2398
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,3 +54,5 @@ declare global { | |
| ): this; | ||
| } | ||
| } | ||
|
|
||
| export type { EvtFn }; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,35 +2,250 @@ import type { InputBinding } from "../bindings/input/inputBinding"; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { OutputBindingAdapter } from "../bindings/outputAdapter"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { EventPriority } from "../inputPolicies/inputPolicy"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { ErrorsMessageValue } from "../shiny/shinyapp"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { EvtFn } from "./jQueryEvents"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import $ from "jquery"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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 | |
| /** | |
| * 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
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.
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.
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.
Done in 10c2398
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.
Why any here? Typically unknown is safer.
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.
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.
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.
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.
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.
Same question here about any.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| import $ from "jquery"; | ||
| import { triggerFileInputChanged } from "../events/inputChanged"; | ||
| import { $escape } from "../utils"; | ||
| import type { ShinyApp } from "../shiny/shinyapp"; | ||
| import { getFileInputBinding } from "../shiny/initedMethods"; | ||
| import { EventInputChanged } from "../events/shinyEvents"; | ||
|
|
||
| type JobId = string; | ||
| type UploadUrl = string; | ||
|
|
@@ -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( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is much nicer |
||
| this.id, | ||
| fileInfo, | ||
| getFileInputBinding(), | ||
| this.el, | ||
| "shiny.fileupload", | ||
| document | ||
| ); | ||
| const inputChangedEvent = new EventInputChanged({ | ||
| name: this.id, | ||
| value: fileInfo, | ||
| el: this.el, | ||
| binding: getFileInputBinding(), | ||
| inputType: "shiny.fileupload", | ||
| }); | ||
| inputChangedEvent.triggerOn(document); | ||
|
|
||
| this.makeRequest( | ||
| "uploadEnd", | ||
|
|
@@ -245,7 +245,7 @@ class FileUploader extends FileProcessor { | |
| this.$bar().text("Upload complete"); | ||
| // Reset the file input's value to "". This allows the same file to be | ||
| // uploaded again. https://stackoverflow.com/a/22521275 | ||
| $(evt.el as HTMLElement).val(""); | ||
| $(inputChangedEvent.el as HTMLElement).val(""); | ||
| }, | ||
| (error) => { | ||
| this.onError(error); | ||
|
|
||
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.
See comments below on this. I'd imagine we can unset this again. Might require some assertions but that's probably safer anyways.