-
Notifications
You must be signed in to change notification settings - Fork 7
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
Various JSONP upgrades #42
Conversation
let o = { foo: 'bar' }, | ||
queryParamName = overrideQueryParamName || 'callback', | ||
body = `/**/ typeof fooFunction === 'function' && fooFunction(${JSON.stringify(o)});`; | ||
const test = ( |
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'm not too thrilled with the increase in number of code paths through this test function. However, don't have any good ideas to improve the situation at the moment. Thoughts?
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.
Yeah, this went from bad to atrocious. Couldn't the expectations
, at least, have been prevented by using extenders? That was the point of the test extender.
d558ca1
to
9e86d2a
Compare
expect(response._isValidJSONPCallback('callback')).to.strictlyEqual(true); | ||
expect(response._isValidJSONPCallback('_abc')).to.strictlyEqual(true); | ||
expect(response._isValidJSONPCallback('$')).to.strictlyEqual(true); | ||
expect(response._isValidJSONPCallback('funcs[123]')).to.strictlyEqual(true); |
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.
Can we add more tests? For example, I don't see anything with a .
, and we should not only test single cases, but mixed cases too (i.e. _
and $
and [..]
and combinations thereof).
let o = { foo: 'bar' }, | ||
queryParamName = overrideQueryParamName || 'callback', | ||
body = `/**/ typeof fooFunction === 'function' && fooFunction(${JSON.stringify(o)});`; | ||
const test = ( |
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.
Yeah, this went from bad to atrocious. Couldn't the expectations
, at least, have been prevented by using extenders? That was the point of the test extender.
9e86d2a
to
eff35d7
Compare
eff35d7
to
5b5b7d9
Compare
This PR is to address: