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): resolve AsyncLocalStorage Illegal Invocation errors #22

Merged
merged 1 commit into from
Nov 19, 2019

Conversation

agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Nov 19, 2019

  • I (and apparently most mst-persist users) don't use localStorage, so
    this went undetected for a while until someone reported a bug

    • and I finally hit upon it myself when adding tests shortly after,
      which added a solid reproduction as well as a blocker on it
  • it made localStorage unusable as all of AsyncLocalStorage's calls
    would give Illegal Invocation errors

    • see https://stackoverflow.com/q/41126149/3431180 for more details
      on this error
      • using .call did not work, not on
        callWithPromise.call(window, ...) nor func.call(window, ...args)
      • similarly window.localStorage.func.bind(window) did not work
      • so instead used wrapper functions as an alternative that didn't
        feel too convoluted
        • using window.localStorage[funcKey] in callWithPromise did not
          quite feel right, wrapper functions felt better

Fixes #18 . Split out of #21 -- was reproduced while creating tests and is needed for tests to pass as well. The fix for this was tested locally against #21, so can be confident it works.

Copy link
Owner Author

@agilgur5 agilgur5 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 now! (The addition of the comment with a link to the issue was the last thing I amended).

Think the commit message should be edited to say "until someone filed an issue for it". Also there's a typo, should be "hit upon it"

@agilgur5 agilgur5 force-pushed the fix-illegal-invocation branch 2 times, most recently from 3bc3729 to 2ceb65b Compare November 19, 2019 04:49
- I (and apparently most mst-persist users) don't use localStorage, so
  this went undetected for a while until someone reported a bug
  - and I finally hit upon it myself when adding tests shortly after,
    which added a solid reproduction as well as a blocker on it

- it made localStorage unusable as all of AsyncLocalStorage's calls
  would give Illegal Invocation errors
  - see https://stackoverflow.com/q/41126149/3431180 for more details
    on this error
    - using .call did not work, not on
      callWithPromise.call(window, ...) nor func.call(window, ...args)
    - similarly window.localStorage.func.bind(window) did not work
    - so instead used wrapper functions as an alternative that didn't
      feel too convoluted
      - using window.localStorage[funcKey] in callWithPromise did not
        quite feel right, wrapper functions felt better
@agilgur5 agilgur5 force-pushed the fix-illegal-invocation branch from 2ceb65b to 81b0927 Compare November 19, 2019 04:50
@agilgur5 agilgur5 merged commit efb1cf6 into master Nov 19, 2019
@agilgur5 agilgur5 deleted the fix-illegal-invocation branch November 19, 2019 04:52
@agilgur5 agilgur5 added the kind: bug Something isn't working properly or is fixed label Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly or is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using AsyncLocalStorage, "Illegal invocation" error occurs
1 participant