Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Set this.client to null before calling client.close() #829

Closed
wants to merge 2 commits into from

Conversation

ibash
Copy link

@ibash ibash commented Nov 9, 2020

client.close can sometimes lead to a synchronous call to close. This
leads to the error:
[TypeError: Cannot set property 'onopen' of null]

This change sets client to null before calling onClose, so that the
second synchronous call does not try to cleanup the client.

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

client.close can sometimes lead to a synchronous call to close. This
leads to the error:
[TypeError: Cannot set property 'onopen' of null]

This change sets client to null before calling onClose, so that the
second synchronous call does not try to cleanup the client.
@ghost
Copy link

ghost commented Nov 16, 2020

Should fix #808

@ghost ghost mentioned this pull request Jan 22, 2021
@tyagow
Copy link

tyagow commented Aug 12, 2021

@ibash sorry to bother you, any estimation on fixing this conflict? I am more than happy to help just not sure how...
Facing this issue right now and I don't want to patch the package...

@ibash
Copy link
Author

ibash commented Aug 12, 2021

@tyagow Unfortunately there's nothing I can do, it's up to the apollo team to merge it and it seems like they've abandoned this package :(

Your best bet is patching the package, happy to help you set that up if need be. But in general:

  1. Follow setup instructions here
  2. Edit node_modules/subscriptions-transport-ws/dist/client.js with these changes
  3. Run npx patch-package subscriptions-transport-ws

@ibash
Copy link
Author

ibash commented Aug 12, 2021

@glasser any chance this can be merged?

@tyagow
Copy link

tyagow commented Aug 13, 2021

@tyagow Unfortunately there's nothing I can do, it's up to the apollo team to merge it and it seems like they've abandoned this package :(

Your best bet is patching the package, happy to help you set that up if need be. But in general:

  1. Follow setup instructions here
  2. Edit node_modules/subscriptions-transport-ws/dist/client.js with these changes
  3. Run npx patch-package subscriptions-transport-ws

Thanks for the instructions, I am using this approach.

@ml242
Copy link

ml242 commented Aug 16, 2021

This package is used by Apollo Client which makes the errors extra irritating as I would have to fork and pin both packages.

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

Successfully merging this pull request may close these issues.

4 participants