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 event type #1923

Merged
merged 4 commits into from
Nov 18, 2019
Merged

add event type #1923

merged 4 commits into from
Nov 18, 2019

Conversation

ofhope
Copy link
Contributor

@ofhope ofhope commented Oct 30, 2019

Casting event param to Event type rather than string. This offers stricter parameter checking avoiding fat fingering and code completion.

@goto-bus-stop
Copy link
Contributor

Thanks for the PR! I'd really like to do this, but I'm not sure if we can. Plugins can add new event types, some of which are user-facing too (like dashboard:modal-closed).

We could do it now by adding like all the plugin event names to this list, but it should be user-extensible so third party plugins can also work :/

@ofhope
Copy link
Contributor Author

ofhope commented Nov 5, 2019

Ahhh yeah I didn't think about plugins potentially creating their own.
We can add string type to the end of the event type. This wouldn't be any different to it is now but would offer code completion in a TS environment.

Heres an example
http://www.typescriptlang.org/play/?ssl=3&ssc=24&pln=3&pc=46#code/C4TwDgpgBAyg9gWwgUQG4QHbCgXigIgGMAbAS0IGt8oAfAgI2IFcAnaugZ2BdIwHMA3ACghhOBi5QAZnDgAueEjSZsefABMAhhwAW9OJpbq5COFuIBaEnA4R1+YWInZ6hhYhTosuAiXJVhISA

Alternatively for pure JS I was wondering about defining a constant hash for events. Personally I find it easier to read. Also simplifies renaming of events, and centralises events in one place hopefully making it more self documenting.

const Event = {
   CANCEL_ALL: "cancel-all"
}

this.emit(Event.CANCEL_ALL)

@goto-bus-stop
Copy link
Contributor

Autocomplete is an interesting point, I didn't think of that as i don't have it set up 🙃

if you would like to add the | string variant to this PR, we would be good to go 👍

@ofhope
Copy link
Contributor Author

ofhope commented Nov 11, 2019

Done. I had to add do some additional typing to get intellisense to work.
It doesn't work as intuitively as I'd thought, you can't just make union type of string :(
Some discussion about it here
microsoft/TypeScript#29729 (comment)

Also trimmed down the verbosity of TS tests since we're accepting string I only tested a few main event types and custom events.

@goto-bus-stop goto-bus-stop merged commit 9abb4e2 into transloadit:master Nov 18, 2019
@goto-bus-stop
Copy link
Contributor

merci merci!

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.

2 participants