Skip to content

Restore block prop for BlockListBlock filter #12943

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
Merged
youknowriad merged 4 commits into from
Dec 17, 2018
Merged

Restore block prop for BlockListBlock filter #12943

youknowriad merged 4 commits into from
Dec 17, 2018

Conversation

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Dec 17, 2018

closes #12938

Plugins used to be able to access the block prop in the BlockListBlock filter which was inadvertently removed when we moved the filter. The intention for this filter was to only allow plugins to use clientId and with this, and using selectors they can retrieve any information about the blocks.

This PR restores the block props for backward compatibility purpose.

Testing instructions

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Dec 17, 2018
@youknowriad youknowriad added this to the 4.7 milestone Dec 17, 2018
@youknowriad youknowriad self-assigned this Dec 17, 2018
@youknowriad youknowriad requested a review from a team December 17, 2018 09:27
@youknowriad youknowriad force-pushed the fix/restore-block-prop branch from c167be5 to 759c0e5 Compare December 17, 2018 09:33
jorgefilipecosta
jorgefilipecosta reviewed Dec 17, 2018
View reviewed changes
gziolo
gziolo previously approved these changes Dec 17, 2018
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This fix looks legit, it would be nice to get confirmation from @aduth whether using memize doesn't introduce any issues. I sense that we should be using Weak Map instead but I'm also not quite sure how it would look implementation wise :)

@gziolo gziolo requested a review from aduth December 17, 2018 13:18
gziolo
gziolo reviewed Dec 17, 2018
View reviewed changes
aduth
aduth reviewed Dec 17, 2018
View reviewed changes
@youknowriad youknowriad force-pushed the fix/restore-block-prop branch from 0fda510 to 8ed5f80 Compare December 17, 2018 14:33
@gziolo gziolo dismissed their stale review December 17, 2018 14:41

It was updated

@gziolo
Copy link
Member

gziolo commented Dec 17, 2018

Moving forward, we need to rethink how to take over control what can be passed to the filtered component. Ideally, we should seek a way to abstract away all usages of filters in Gutenberg. It never occurred to me before how bad it can become if the number of props passed to the filter introduced withFilter grow over time.

@youknowriad
Copy link
Contributor Author

youknowriad commented Dec 17, 2018

Also, we should explore separately how to catch errors in withFilter callbacks and ignore the filters entirely instead of breaking the whole editor.

@youknowriad youknowriad force-pushed the fix/restore-block-prop branch from 99e4d9c to 61df553 Compare December 17, 2018 15:26
@youknowriad
Copy link
Contributor Author

youknowriad commented Dec 17, 2018

Tests are funky at the moment. I'm going to merge while we figure out what's wrong with our test setup.

@youknowriad youknowriad merged commit d9257b1 into master Dec 17, 2018
@youknowriad youknowriad deleted the fix/restore-block-prop branch December 17, 2018 15:38
youknowriad added a commit that referenced this pull request Dec 18, 2018
@youknowriad
Restore block prop for BlockListBlock filter (#12943)
410b622 
* Restore block prop for BlockListBlock filter

* Restore hook order

* getBlock with memory leak

* Add clientId and isValid to the block prop
@aduth aduth mentioned this pull request Dec 18, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gziolo gziolo gziolo left review comments

@jorgefilipecosta jorgefilipecosta jorgefilipecosta left review comments

@aduth aduth aduth approved these changes

Assignees

@youknowriad youknowriad

Labels

[Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

4.7

Development

Successfully merging this pull request may close these issues.

Removing getBlock from the BlockListBlock component breaks back compat

5 participants

@youknowriad @gziolo @aduth @jorgefilipecosta