-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
All Web- and ServiceWorkers reliably destroyed by new cognito-identity-js
#6868
Comments
cognito-identity-js
cognito-identity-js
cognito-identity-js
cognito-identity-js
For reference, it looks like supabase/postgrest-js#109 moved to use cross-fetch which doesn't appear to have the issue when run within workers so using that over the current solution is one option. |
FYI, the offending problem in the The problem in |
Thanks for reporting this @ngault, @nickarocho can we look at this bug and see about the upgrade path? |
Hi @ngault, I started looking into this and was wondering if this is still an issue for you? The Since this should bump the dependency to any minor version (via the
If this is still a problem for you we will open a PR with the minor version bump, otherwise I believe we can close this issue. Thanks! |
The problem was fixed within a few days of my reporting it 8 month as ago. @ericclemmons took the lead. |
Got it, thanks for the follow up! |
This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Describe the bug
With the introduction of
isomorphic-unfetch
toamazon-cognito-identity-js/src/Client.js
atamplify-js/packages/amazon-cognito-identity-js/src/Client.js
Line 1 in 7cbed47
window not found
. Moreover, the inclusion of file in a JS bundle instantly destroys the browser's Worker environment, with other fatal exceptions soon to follow.This is because
isomorphic-unfetch
depends onunfetch
, which fails to check forself
(i.e., a Worker environment), and, thereby, replaces the Worker environment's built-infetch
implementation with a fetch polyfill that, incorrectly, assumeswindow
is present.developit/unfetch#104
To fully appreciate the severity of this bug, consider that that the standardized worker implementation of the
fetch
spec (in Chrome it's >> 100,000 lines, and includes proxying, cacheing, etc) is replaced by a 500 line polyfill the moment the worker loads the Amplify JS bundle, and executesimport "isomorphic-unfetch"
.Unfortunately, the
unfetch
project is not actively maintained, with a straightforward PR to fix the problem being outstanding almost 2 years:developit/unfetch#109
The problem CAN be fixed without impacting any other Amplify code by either:
1. replacing
unfetch
with afetch
polyfill that correctly detects all environments,OR
2. checking for
self
on the 1st line ofClient.js
, and, if a Worker is detected, not importingisomorphic-fetch
and overwriting the Worker global namespaceTo Reproduce
Based on the self-evident nature of the problem and its documentation in the
isomorphic-unfetch
/unfetch
repo, reproducing the behavior in a Worker using Amplify should not be necessary... but, if our process nonetheless requires reproducing, do this:From within a Web or Service Worker, call any method in
@aws-amplify/auth
(which will, in turn invokeamazon-cognito-identity-js/src/Client.js
) or any other Amplify function dependent on@aws-amplify/auth
.The
/amazon-cognito-identity-js/__tests__/
does not currently consider workers (nor SSR). Similarly, there is no testing for authentication in workers in core... all of which explains why this problem wasn't previously identified.To configure a worker to use Auth, you'll need to provide an implementation of a the
ICognitoStorage
interface with your Config with something like:The custom storage interface is required because workers don't support LocalStorage, Amplify's default store. Numerous examples for memory and IndexedDB stores can be found with a Google search.
The text was updated successfully, but these errors were encountered: