-
Notifications
You must be signed in to change notification settings - Fork 3.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
fix: handle promises rejected with undefined gracefully #29454
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// /~https://github.com/cypress-io/cypress/issues/23927 | ||
describe('issue 23927', { browser: '!webkit' }, () => { | ||
it('Fails gracefully if origin page throws undefined', () => { | ||
cy.visit('http://barbaz.com:3500/fixtures/generic.html') | ||
cy.origin('http://foobar.com:3500', () => { | ||
Cypress.on('uncaught:exception', (err, runnable) => { | ||
expect(err.message).to.contain('An unknown error has occurred: undefined') | ||
|
||
return false | ||
}) | ||
|
||
cy.visit('http://foobar.com:3500/fixtures/throws-undefined.html') | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<title>Promise.reject undefined</title> | ||
</head> | ||
|
||
<body> | ||
<h1> I am going to reject undefined in the document </h1> | ||
</body> | ||
<script> | ||
Promise.reject(undefined) | ||
</script> | ||
</html> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -188,23 +188,34 @@ const appendErrMsg = (err, errMsg) => { | |||||
}) | ||||||
} | ||||||
|
||||||
const makeErrFromObj = (obj) => { | ||||||
const makeErrFromObj = (obj: any) => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this type be better?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Another issue: this typing does not match against objects without Maybe use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in this case we might not be sure what a promise can reject with, so I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we really improve the type checking, since we have to accept anything? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can improve it slightly where we either expect a string, or an object with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tried to make this a bit more type safe in b294040 |
||||||
if (_.isString(obj)) { | ||||||
return new Error(obj) | ||||||
} | ||||||
|
||||||
const err2 = new Error(obj.message) | ||||||
if (_.isObject(obj) && _.isString((obj as any).message) && _.isString((obj as any).name)) { | ||||||
obj = obj as { | ||||||
message: string | ||||||
name: string | ||||||
stack?: string | ||||||
} | ||||||
|
||||||
err2.name = obj.name | ||||||
err2.stack = obj.stack | ||||||
const err2 = new Error(obj.message) | ||||||
|
||||||
_.each(obj, (val, prop) => { | ||||||
if (!err2[prop]) { | ||||||
err2[prop] = val | ||||||
} | ||||||
}) | ||||||
err2.name = (obj as any).name | ||||||
err2.stack = (obj as any).stack | ||||||
|
||||||
_.each(obj, (val, prop) => { | ||||||
if (!err2[prop]) { | ||||||
err2[prop] = val | ||||||
} | ||||||
}) | ||||||
|
||||||
return err2 | ||||||
} | ||||||
|
||||||
return err2 | ||||||
// handle all other errors gracefully (e.g. a promise is rejected with undefined) | ||||||
return new Error(`An unknown error has occurred: ${obj}`) | ||||||
} | ||||||
|
||||||
const makeErrFromErr = (err, options: any = {}) => { | ||||||
|
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.
@AtofStryker If you have a global
Cypress.on
handler, will it not catch any uncaught:exceptions from origin? Is this documented somewhere?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.
I believe it is but I am not sure where, which tells me if it is, it is not obvious.
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.
@AtofStryker Can you update the docs to include this information here: https://docs.cypress.io/api/cypress-api/catalog-of-events#Uncaught-Exceptions I think this was the main problem people were running into since they just wanted to ignore it.
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.
added here cypress-io/cypress-documentation#5821