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

useSelect crashes blocks if a registered store doesn't implement unsubscribe #27631

Closed
fullofcaffeine opened this issue Dec 10, 2020 · 5 comments · Fixed by #27634
Closed

useSelect crashes blocks if a registered store doesn't implement unsubscribe #27631

fullofcaffeine opened this issue Dec 10, 2020 · 5 comments · Fixed by #27634
Assignees
Labels
[Package] Data /packages/data [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@fullofcaffeine
Copy link
Member

fullofcaffeine commented Dec 10, 2020

Describe the bug

useSelect might bubble up a TypeError (unsubscribe is not a function) that ends up crashing a block in the editor. This happens because it calls unsubscribe unconditionally without taking into account stores that do not return unsubscribe from their subscribe function.

This can happen with custom generic stores, where the custom subscribe function implementation might not return the unsubscribe function since, by definition, it's optional (see docs).

To reproduce

Steps to reproduce the behavior:

  1. Clone the CoBlocks, switch to the 2.5.3 tag;
  2. Build your local version of CoBlocks by running yarn && yarn build;
  3. In another top-level directory, clone Gutenberg, set it to the v9.5.1 tag, build it;
  4. Go back to the CoBlocks directory and add this .wp-env.json: https://gist.github.com/fullofcaffeine/aa4c76e5fad53f3ac52366bff3a85e44;
  5. Make sure you have wp-env installed globally;
  6. Start your local WordPress instance with CoBlocks 2.5.3 and Gutenberg 9.5.1 by running wp-env start .;
  7. Start a new post (http://localhost:8889/wp-admin/post-new.php);
  8. Add or focus a paragraph block, type something, deselect it by clicking something else;
  9. You should see the block crash and the error in the browser's JS console.

This seems to happen with other blocks too. I smoke-tested columns, quote, list, and they all crashed.

Expected behavior

Blocks don't crash and work when the user adds/sets them up if a generic wp-data store that doesn't implement an unsubscribe function is registered.

Screencast

gbcoblocksissue

Editor version (please complete the following information):

  • WordPress version: 5.5.3
  • Does the website has Gutenberg plugin installed or is it using the block editor that comes by default?: gutenberg plugin
  • If the Gutenberg plugin is installed, which version is it? 9.5.1

Desktop (please complete the following information):

  • OS: Ubuntu Linux 18.04
  • Browser: Firefox 84.0b8 (64-bit) and Chrome 87.0.4280.66 (Official Build) (64-bit)

Additional context

@fullofcaffeine fullofcaffeine changed the title useSelect crashes blocks if a registered store doesn't implement unsubscribe useSelect crashes blocks if a registered store doesn't implement unsubscribe Dec 10, 2020
@ockham
Copy link
Contributor

ockham commented Dec 10, 2020

cc/ @kevin940726 @adamziel @youknowriad

@kevin940726
Copy link
Member

I indeed didn't take generic store into account, thanks! I'll submit a PR soon.

@youknowriad
Copy link
Contributor

I believe the issue here is not in Gutenberg itself. The unsubscribe is mandatory for stores. That said, since this was not forced in code previously, we should be more permissive but this is a case where a warning might be good to potentially remove the check in the future.

@sirreal sirreal added [Package] Data /packages/data [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Dec 10, 2020
@sirreal
Copy link
Member

sirreal commented Dec 10, 2020

The unsubscribe is mandatory for stores.

The documentation clearly states that unsubscribe is optional for generic stores:

Behaves as Redux subscribe with the following differences:

  • Doesn't have to implement an unsubscribe, since the registry never uses it.

Changing that is a breaking change and definitely needs some notice and warning, probably a dev note.

@youknowriad
Copy link
Contributor

Mmm :) Good catch, I wasn't aware of that and it doesn't really make sense for me :P but you're right if we do change, we need all the docs and communication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
5 participants