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

chore: Use fetch as default for browsers instead of reqwest #199

Merged
merged 8 commits into from
Apr 15, 2019

Conversation

manueltanzi-okta
Copy link
Contributor

@manueltanzi-okta manueltanzi-okta commented Mar 27, 2019

Description

  • okta-auth-js use reqwest by default as a http request library, this change make it use cross-fetch instead.
  • All the test were using jquery as a request library, converted it to use cross-fetch instead.

Note: Will now try with alpha versions if it works in okta-signing-widget + okta-core before merging.
Note2: Haven't removed the possibility to use jquery as a request library (and the jquery dependency) because it would be a breaking change. But we can easily remove it next time we bump major version.

Reviewers

@haishengwu-okta @jmelberg-okta @rchild-okta

@@ -33,8 +33,8 @@ function fetchRequest(method, url, args) {
}
return result;
};
if (response.headers.get('Accept') &&
response.headers.get('Accept').toLowerCase().indexOf('application/json') >= 0) {
if (response.headers.get('Content-Type') &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected, should be Content-Type in response.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking - now that we're using cross-fetch in our tests, are we able to catch this error?

Also, did notice that we're not mocking out the response.text() function here - do we need to? In general, when will be have a response that's not application/json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, now we are able to catch that!

fyi, response.text() was working properly anyway for our flow, since we always do a JSON.parse in http https://github.com/okta/okta-auth-js/blob/master/lib/http.js#L60, this is why I haven't notice that during manual testing.

For what I have seen all our response should be application/json, so maybe should I remove the if/else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable if we're catching it downstream. Would also be curious why we have that code to handle it in the first place - my hunch is that at some point we'd occasionally return an error that was not a json response. This might be fixed now though.

jest.server.json Outdated Show resolved Hide resolved
Copy link
Contributor

@haishengwu-okta haishengwu-okta left a comment

Choose a reason for hiding this comment

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

curious how did the cross browser testing go?

test/util/util.js Show resolved Hide resolved
@haishengwu-okta
Copy link
Contributor

some thoughts on refactor dirctory

  • remove /none
  • remove /reqwest
  • move /fetch into /lib

@manueltanzi-okta
Copy link
Contributor Author

@haishengwu-okta good points! But as for /jquery, planning on doing those later when we bump major version, since they would be breaking changes (except for the third bullet).
https://github.com/okta/okta-auth-js/blame/master/README.md#L81

@@ -33,8 +33,8 @@ function fetchRequest(method, url, args) {
}
return result;
};
if (response.headers.get('Accept') &&
response.headers.get('Accept').toLowerCase().indexOf('application/json') >= 0) {
if (response.headers.get('Content-Type') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking - now that we're using cross-fetch in our tests, are we able to catch this error?

Also, did notice that we're not mocking out the response.text() function here - do we need to? In general, when will be have a response that's not application/json?

jest.browser.json Outdated Show resolved Hide resolved
test/util/util.js Show resolved Hide resolved
@manueltanzi-okta
Copy link
Contributor Author

@haishengwu-okta Sorry for the wait, took long time to get the cross browser testing completed.

But, good news! They all passed!

Screen Shot 2019-04-01 at 10 07 06 AM

Screen Shot 2019-04-01 at 10 06 58 AM

@manueltanzi-okta manueltanzi-okta force-pushed the manueltanzi-use-fetch-in-browser branch from cf1a8ec to b5e3dc0 Compare April 1, 2019 20:47
Copy link
Contributor

@rchild-okta rchild-okta left a comment

Choose a reason for hiding this comment

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

Looks great 🚀

Copy link
Contributor

@haishengwu-okta haishengwu-okta left a comment

Choose a reason for hiding this comment

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

looks good to me. would be great if we can get tested at embedded browser and 🚢

lib/token.js Outdated Show resolved Hide resolved
@manueltanzi-okta
Copy link
Contributor Author

manueltanzi-okta commented Apr 8, 2019

@haishengwu-okta fyi, @vijetmahabaleshwar-okta just completed the embedded browser testing (thank you Vijet!), everything good 👍

https://oktainc.atlassian.net/browse/OKTA-219259

@vijetmahabaleshwar-okta
Copy link
Contributor

Completed sanity testing your branch. Everything looks good.
Tested outlook, skype & word o365 apps for sign-in with and without MFA. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants