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.

Sign up for GitHub

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

Jump to bottom

Do not use AbortController in the worker if not available #679

Merged
merged 7 commits into from
Jan 5, 2021
Merged

Do not use AbortController in the worker if not available #679

merged 7 commits into from
Jan 5, 2021

Conversation

stevehobbsdev
Copy link
Contributor

@stevehobbsdev stevehobbsdev commented Dec 18, 2020 •
edited
Loading

This PR excludes the use of AbortController inside the web worker if it is
not natively supported by the browser. Currently, this generates errors if it
is not available, but we wouldn't like to polyfill it due to the size.

I have changed the code to allow browsers that support WebWorker but not AbortController to once again use the web worker.

Tested in Safari 10, 11, 12 and Firefox 52.

Fixes #675

@stevehobbsdev stevehobbsdev requested a review from a team as a code owner December 18, 2020 13:58
@stevehobbsdev stevehobbsdev added CH: Changed PR is changing something review:small Small review labels Dec 18, 2020
stevehobbsdev
stevehobbsdev commented Dec 18, 2020
View reviewed changes
src/Auth0Client.ts
@@ -92,8 +92,7 @@ const cacheFactory = (location: string) => {
/**
* @ignore
*/
const supportWebWorker = () =>
!isIE11() && !isSafari10() && !isSafari11() && !isSafari12_0();
const supportWebWorker = () => !isIE11();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frederikprijck is it fair to say we only added these Safari checks to stop it from using the web worker purely because of AbortConrtoller, or were there other factors?

I haven't removed these functions, do you see value in keeping them around or should we remove them?

Copy link
Member

Choose a reason for hiding this comment

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

It was to fix the AbortController: #594

Copy link
Member

Choose a reason for hiding this comment

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

If we revert this, we should test this again on Safari. LMK if you dont have access to BrowserStack, I dont mind testing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I've tested, versions I tested are in the PR

@stevehobbsdev
Copy link
Contributor Author

Fixing tests now

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 18, 2020

This pull request introduces 1 alert when merging 700f3ca into 171dfce - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

stevehobbsdev
stevehobbsdev commented Dec 18, 2020
View reviewed changes
__tests__/Auth0Client/getTokenSilently.test.ts Outdated
Comment on lines 620 to 643
},
{
name: 'Safari 10',
userAgent:
'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.1.2 Safari/603.3.8',
supported: false
},
{
name: 'Safari 11',
userAgent:
'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.1.28 (KHTML, like Gecko) Version/11.0 Safari/604.1.28',
supported: false
},
{
name: 'Safari 12',
userAgent:
'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0.1 Safari/605.1.15',
supported: false
},
{
name: 'Safari 12.1',
userAgent:
'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.2 Safari/605.1.15',
supported: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't completely refactor the test here despite there being only 1 browser to test, but we might add to this later so made sense to me to leave it. What do you think? @frederikprijck

Copy link
Member

@frederikprijck frederikprijck Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I agree! Let's keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codecov says otherwise 😂

Copy link
Member

Choose a reason for hiding this comment

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

Ye we need to add one with supported: true .. 😬 Or rework it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can leave the Safari 12.1 test in, would that help?

Copy link
Member

Choose a reason for hiding this comment

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

I think that could help. Wouldn't make much sense to some degree tho. Because why test only safari 12.1, and not other versions or firefox or ... ?

Copy link
Contributor Author

@stevehobbsdev stevehobbsdev Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've added a known supported browser in this test, Chrome 74, just to give us that positive and negative angle.

frederikprijck
frederikprijck reviewed Jan 5, 2021
View reviewed changes
src/worker/token.worker.ts

port.postMessage({
error: "Timeout when executing 'fetch'"
});
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly for the benefit of the test, to be able to determine that the failure was because of a timeout, but could also be a useful error message on the client regardless. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. It rejects the promise using the same error we already have incase it timesout based on the setTimeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't do.. if the timeout is hit, no response will be returned from Promise.race and falls into the next block that checks for an empty response, rather than falling into the catch. I could be mistaken; I tried to test for that here: https://github.com/auth0/auth0-spa-js/blob/fix/abort-controller/__tests__/token.worker.test.ts#L146

@stevehobbsdev stevehobbsdev merged commit aa2cd76 into master Jan 5, 2021
@stevehobbsdev stevehobbsdev deleted the fix/abort-controller branch January 5, 2021 14:48
@stevehobbsdev stevehobbsdev added this to the vNext milestone Jan 7, 2021
@stevehobbsdev stevehobbsdev mentioned this pull request Jan 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frederikprijck frederikprijck frederikprijck approved these changes

Assignees
No one assigned
Labels
CH: Changed PR is changing something review:small Small review
Projects
None yet
Milestone
v1.13.6
Development

Successfully merging this pull request may close these issues.

AbortController is not defined
2 participants
@stevehobbsdev @frederikprijck