Skip to content

Clarification on construction and creation of events #372

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

Open
marcoscaceres opened this issue Nov 10, 2016 · 9 comments
Open

Clarification on construction and creation of events #372

marcoscaceres opened this issue Nov 10, 2016 · 9 comments

Comments

@marcoscaceres
Copy link
Member

Could you kindly provide a bit more guidance with respect to creation and construction of events? Right now, one can do:

Create an event using FooBar interface, and let x be the result.

But, the algorithm has not called x's constructor - not has it set its type.

If I'm using "create an event", do I then call the constructor straight after with the type? Why not allow type to be provided with "create an event"?

Maybe the spec could provide an example of the ideal order for these.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Nov 10, 2016

Also, if I want to have cancelable as true as default, do I need to have that as an override on FooBarEventInitDict? Like:

dictionary FooBarEventInitDict : EventInit {
    bool cancelable = true;
};

@marcoscaceres
Copy link
Member Author

s/if I want to have cancelable as true/if I want to have cancelable as true as default

@domenic
Copy link
Member

domenic commented Nov 10, 2016

Examples and explanations are always good; we should add some.

In general "create an event" should be used very rarely, in specialized situations where you need to separate event creation from event dispatch, or want to customize the realm the event is created in. Fire an event is usually more appropriate.

But, the algorithm has not called x's constructor

Sure, but why would you need to? "create an event" runs all the constructor logic. Compare create an event and event constructor

  • not has it set its type.

Yep. You can do that yourself pretty easily though. "Initialize event's type attribute to "foo"".

Why not allow type to be provided with "create an event"?

It's cleaner to explicitly state what you're setting, than passing it in as an unnamed argument. (And if you pass it in as a named argument, you aren't really saving anything.)

Also, if I want to have cancelable as true as default

You should never override the default of basic properties of events like cancelable. That would be very unexpected. And what would be the point of making user-created events cancelable by default, since you don't know whether the user will react to cancelation?

However, you can just ensure that whenever you fire or dispatch an event yourself in your spec, you initialize cancelable to true, as in the "fire an event" example. Be sure to use the return value, as in the example.

(Speaking of which, @annevk, the doAction example should probably be marked as cancelable, otherwise doAction is always true, right?)

@marcoscaceres
Copy link
Member Author

Thanks @domenic!

In general "create an event" should be used very rarely, in specialized situations where you need to separate event creation from event dispatch, or want to customize the realm the event is created in. Fire an event is usually more appropriate.

Ok, maybe we should clarify the above in the spec? In particular, warn people that they should really go and just use "fire an event". In fire an event, "eventConstructor" should maybe be renamed "eventInterface" as to avoid confusion (it confused me initially as I thought it needed a constructor function).

You should never override the default of basic properties of events like cancelable. That would be very unexpected. And what would be the point of making user-created events cancelable by default, since you don't know whether the user will react to cancelation?

Yeah, true.

@marcoscaceres
Copy link
Member Author

So, more concretely, in relation to creation, I have the following:

[Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
interface BeforeInstallPromptEvent : Event {
    Promise<PromptResponseObject> prompt();
};

dictionary BeforeInstallPromptEventInit : EventInit {
  AppBannerPromptOutcome userChoice;
};

dictionary PromptResponseObject {
  AppBannerPromptOutcome userChoice;
};

In the create algo, the spec says:

For each dictionary member present in eventInitDict, find the attribute on event whose identifier matches the key of the dictionary member and then set the attribute to the value of that dictionary member.

But BeforeInstallPromptEvent doesn't have a matching "userChoice" for BeforeInstallPromptEventInit.userChoice.

My intention was so developers could construct these events and have .prompt() resolve immediately for testing purposes:

var x = new BeforeInstallPromptEvent("beforeinstalmprompt", {userChoice: "accepted"})
x.prompt().then(({userChoice}) => `The user selected: ${userChoice}`)

But not realizing that this is incompatible with the create algo 😢. I should probably drop BeforeInstallPromptEventInit, right?

@domenic
Copy link
Member

domenic commented Nov 10, 2016

Ok, maybe we should clarify the above in the spec? In particular, warn people that they should really go and just use "fire an event".

That's kind of implied by the spec's "Create an event is meant to be used by other specifications which need to separately create and dispatch events, instead of simply firing them," but it could be clearer I suppose.

But BeforeInstallPromptEvent doesn't have a matching "userChoice" for BeforeInstallPromptEventInit.userChoice.

Oh, then it's an invalid event.

I should probably drop BeforeInstallPromptEventInit, right?

Yeah, it's not really OK for an event to be able to synchronously provide a value but not synchronously read it back. So either remove the ability to initialize it with a given userChoice, or just add a userChoice property to BeforeInstallPromptEvent.

@marcoscaceres
Copy link
Member Author

Thanks for the clarification... and hugs during these shitty times.

@annevk
Copy link
Member

annevk commented Nov 10, 2016

Reopening since it seems some work might be needed still.

@domenic
Copy link
Member

domenic commented Nov 10, 2016

So I think the takeaways are:

  • Mention the 1:1 correspondence between event init dict and event properties as part of Add a section on defining a new event #364
  • Maybe explicitly state that you cannot define your own constructor for an event
  • Explicitly mention that create + dispatch should be rare and fire is more often what you want; perhaps give an example of when create + dispatch is appropriate
  • eventConstructor -> eventInterface?
  • Fix doAction example to be cancelable

Anything else that would be helpful?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants