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

Fix: Error thrown when fetch argument is not a string or a Request object. #1069

Merged
merged 1 commit into from
Oct 3, 2017

Conversation

vierno
Copy link
Contributor

@vierno vierno commented Oct 2, 2017

Hello,

When using fetch and the first parameter of fetch is undefined, raven is throwing an error:

> fetch(undefined)
< Uncaught TypeError: Cannot read property 'url' of undefined

This fix covers the cases where the first arg of fetch is neither a string or a Request object.

src/raven.js Outdated
var method = 'GET';
var url;

if (typeof fetchInput === 'string') {
url = fetchInput;
} else {
} else if (
typeof 'Request' !== undefined &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always return true, as typeof 'Request' is always string.
Use 'Request' in _window to verify that Request is available.

src/raven.js Outdated
url = fetchInput.url;
if (fetchInput.method) {
method = fetchInput.method;
}
} else {
url = String(fetchInput);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use url = '' + url instead? We use this syntax throughout the codebase.

@kamilogorek
Copy link
Contributor

kamilogorek commented Oct 3, 2017

Hey!
Thanks for contributing. There are just two things that need some changes.
We also need a test case for this scenario.

@vierno
Copy link
Contributor Author

vierno commented Oct 3, 2017

Hello, @kamilogorek ,

I've made the requested changes and updated the pull request.

Thank you!

@kamilogorek kamilogorek merged commit 63474f8 into getsentry:master Oct 3, 2017
@kamilogorek
Copy link
Contributor

Thanks!

@benvinegar
Copy link
Contributor

@vierno so, I agree Raven shouldn't choke when it gets an argument like this, but I'm curious if there's any actual defined behavior for fetch(123) or fetch(undefined)? The browser lets me do it, but it's not clear what's actually happening.

@vierno
Copy link
Contributor Author

vierno commented Oct 3, 2017

@benvinegar , the browser uses the string representation of the argument as the url. So fetch(undefined) gives the same result of fetch("undefined")

@kamilogorek
Copy link
Contributor

Unfortunately this test is failing on IE10/11/Safari10, but I'll try to fix it. Unfortunately, our setup at the moment doesn't allow to run SauceLabs tests for non-organization members. We're working on fixing this setup as well.

@benvinegar
Copy link
Contributor

the browser uses the string representation of the argument as the url. So fetch(undefined) gives the same result of fetch("undefined")

@vierno – Ah, thanks for the clarification.

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.

3 participants