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

Conform to spec when generating PromiseRejectionEvent #1464

Merged
merged 3 commits into from Oct 6, 2017

Conversation

nornagon
Copy link
Contributor

@nornagon nornagon commented Oct 5, 2017

Fixes #1447

cancelable: true
cancelable: true,
reason: event.reason,
promise: event.promise
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to write this as get promise() { return event.promise; }? I'm unfamiliar with the compilation pipeline for bluebird, and whether that syntax is supported on all the browsers Bluebird targets.

If we wanted to be even more protective, we could do something like:

var reason = event.reason;
var promise = event.promise;
var domEvent = new CustomEvent(name.toLowerCase(), {
  detail: event,
  cancelable: true,
  get reason() { return reason; },
  get promise() { return promise; }
})

or the equivalent with Object.defineProperty

Let me know if that's a desirable change & I can update the PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bluebird actually targets ES3 browsers too (supports IE7 for example) - on the other hand this branch of code isn't reached on these browsers anyway.

@nornagon
Copy link
Contributor Author

nornagon commented Oct 5, 2017

I just realised this doesn't handle the case where CustomEvent isn't defined, will update.

@nornagon
Copy link
Contributor Author

nornagon commented Oct 5, 2017

Updated to use getters. Let me know if I should squash these commits.

promise: event.promise
cancelable: true
};
Object.defineProperties(eventData, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're using this syntax anyway - can you please use the helpers from util that work in ES3 environments for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (I think!)

@petkaantonov
Copy link
Owner

so is it supposed to be data property or getters? it's currently data property

@petkaantonov
Copy link
Owner

checked the spec and its readonly data.. merging

@petkaantonov petkaantonov merged commit b7f2139 into petkaantonov:master Oct 6, 2017
@nornagon nornagon deleted the promise-rejection-event branch October 6, 2017 19:05
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.

None yet

3 participants