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

[dom.js] add ErrorEvent #8336

Closed
wants to merge 3 commits into from
Closed

[dom.js] add ErrorEvent #8336

wants to merge 3 commits into from

Conversation

kegluneq
Copy link
Contributor

@kegluneq kegluneq commented Apr 1, 2020

Summary: There is no ErrorEvent declaration in dom.js. This diff adds ErrorEvent.

MDN documentation: https://developer.mozilla.org/en-US/docs/Web/API/ErrorEvent
whatwg documentation: https://html.spec.whatwg.org/multipage/webappapis.html#the-errorevent-interface
w3.org documentation: https://www.w3.org/TR/html52/webappapis.html#the-errorevent-interface

Summary: There is no `ErrorEvent` declaration in dom.js. This diff adds `ErrorEvent`.

MDN documentation: https://developer.mozilla.org/en-US/docs/Web/API/ErrorEvent
@jamesisaac jamesisaac added the Library definitions Issues or pull requests about core library definitions label Apr 2, 2020
Summary: Add an ErrorEvent test to ensure the ErrorEvent constructs successfully
and its properties adhere to expected types.
@kegluneq
Copy link
Contributor Author

kegluneq commented Apr 2, 2020

It looks like there are test failures, but that these are due to offset changes in the expected test outputs. I did some searching around but couldn't find the process for updating the expected files, if there is one. Do I just apply the diffs that get generated when I run the tests to the relevant .exp files?

Thanks!

@w01fgang
Copy link
Contributor

@kedromelon I couldn't find the way to update that snapshot, and I just removed old snapshot and copied raw output from the terminal after running the test on dom.

Summary: Adding the ErrorEvent test changed line numbers in the .exp files. Update those files with the new values.
@kegluneq
Copy link
Contributor Author

I ended up using patch with the generated .diff files to update after manually verifying that the changes were all line numbers

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mroch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kegluneq merged this pull request in 56b9a80.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Library definitions Issues or pull requests about core library definitions Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants