-
Notifications
You must be signed in to change notification settings - Fork 798
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
Blocks: Deprecations - ensure supports are exported within larger configuration objects. #40070
Conversation
…ues with changes in Gutenberg.
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
…tions, to prevent invalid blocks when updating Jetpack
151a69f
to
a351788
Compare
…xport default object
…export default object for consistency
a351788
to
2fb7982
Compare
…tenberg-incompatibility
Thanks for working on this and for identifying the issue in Gutenberg! This PR may not be necessary as there's an upstream PR now that I think should fix it: WordPress/gutenberg#66849 (if you get a chance, please give it a test) That said, the way that Jetpack Blocks structure the deprecations did look a little unusual to me, so for code quality reasons it might still be worth tidying up? Something I was wondering about is why the
Instead of something like:
Then in each deprecation, something like the following so that it's explicit what the
Would that work? From my perspective the structure of the files in this plugin looks a bit more granular than I'm used to compared to core WP blocks. This is just my first impression from looking at the Jetpack code, though, so please feel free to disregard this comment if there are good reasons why the Jetpack blocks are structured the way they are right now! 🙂 |
Just a quick note of appreciation—thanks so much for flagging this up in Core! |
💯 Yes! This was exactly the kind of testing and feedback we needed for this work, so another big thank you from me! |
Thanks for the quick fix @andrewserong ! It does work for Jetpack, so this PR isn't necessary and I'll close it out. Good question re the current deprecation approach though. It appears that what has happened is that the approach was taken in one instance some time back, and has been copied in several cases since. I'll add a task for us to clean this up and look into the suggestions you've made here, thank you! |
Fixes #40055
Note - this may be better dealt with at the Gutenberg end - comment left here: WordPress/gutenberg#63401 (comment)
Proposed changes:
stabilizeSupports
function in Gutenberg now expects supports to be part of a larger block configuration object that doesn’t have getter-only behavior.stabilizeSupports
expects.Other information:
Jetpack product discussion
#40055
Does this pull request change what data or activity we track or use?
Testing instructions:
To replicate the issue:
/wp-admin/admin.php?page=jetpack#/writing
/wp-admin/site-editor.php
, or a post / page editor in wp-admin.process-block-type.js:153 Uncaught TypeError: Cannot set property supports of #<Object> which has only a getter
.To test the fix: