-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
feat(ext/web): Add reportError() #13799
Conversation
@nayeemrmn I think this is good implementation wise. I want to get some more eyes on it before landing. @crowlKats @lucacasonato please take a look |
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.
Actually, WPTs don't pass with my WPT patch:
Going to run 2 test files.
----------------------------------------
/html/webappapis/scripting/reporterror.any.html
test self.reportError(1) ... ok
test self.reportError(TypeError) ... ok
test self.reportError(undefined) ... failed
test self.reportError() (without arguments) throws ... ok
test self.reportError() doesn't invoke getters ... ok
test stderr:
file result: failed. the event loop run out of tasks during the test
@lucacasonato #13799 (comment). Chrome also fails that right now. We can get compliant FireFox behaviour by adding an internal escape hatch to |
My concern is not that failing test, but the "event loop ran out of tasks" thing. |
Running locally I got varied results... eventually I reproduced this but now it's happening for every test even after I switch |
It seems like it's not related to this PR, but some change in wpt since web-platform-tests/wpt@96f98c4 (deno main) and wpt master is making this happen to every test. |
This reverts commit 708d012.
Discussed with @lucacasonato, he'll try to fix the WPT runner before merging this PR |
@nayeemrmn Can you rebase once more? WPT is updated on |
@lucacasonato Ready |
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.
LGTM!
Sorry this one took so long to land @nayeemrmn. Thank you very much for this work! |
Given #13987 (comment), this seems to be the only other PR which affects networking in the 1.21.0 release. Could this be responsible for #14437? Based on the code changes, I don't really understand how, though … |
It seems to me like |
Blocked on #14159.
Closes #13484.