Update react-resize-aware with use-resize-observer #40509
Conversation
youknowriad
added
the
[Type] Code Quality
|
I'd appreciate some testing on this issue to see if there's no regressions. |
|
Size Change: -118 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Looks like that license is not compatible, it's fine I can definitely write my own hook using |
|
Ok, so it looks like the dependency is actually compatible "MIT" but it references a polypill |
|
Ok, this should be ready for testing. |
package.json
Outdated
| @@ -243,7 +243,7 @@ | |||
| "build:plugin-zip": "bash ./bin/build-plugin-zip.sh", | |||
| "build": "npm run build:packages && wp-scripts build", | |||
| "changelog": "node ./bin/plugin/cli.js changelog", | |||
| "check-licenses": "concurrently \"wp-scripts check-licenses --prod --gpl2 --ignore=@react-native-community/cli,@react-native-community/cli-platform-ios\" \"wp-scripts check-licenses --dev\"", | |||
| "check-licenses": "concurrently \"wp-scripts check-licenses --prod --gpl2 --ignore=@juggle/resize-observer,@react-native-community/cli,@react-native-community/cli-platform-ios\" \"wp-scripts check-licenses --dev\"", | |||
There was a problem hiding this comment.
@juggle/resize-observer is licensed as Apache-2.0 that isn't GPL2 compatible and therefore can't be used in the production code.
There was a problem hiding this comment.
Yes, I can confirm that @youknowriad is correct about the usage of the polyfill in #40509 (comment).
There is no usage in the bundle shipped with the package:
https://unpkg.com/browse/[email protected]/dist/bundle.esm.js
We can always fork this simple utility to avoid future issues with the problematic polyfill.
There was a problem hiding this comment.
I'd be OK with forking into Gutenberg too.
There was a problem hiding this comment.
What's the browser support situation?
There was a problem hiding this comment.
Better than a lot of things we use already https://caniuse.com/resizeobserver (supported in all WP supported browsers)
There was a problem hiding this comment.
So what do we do here? Just merge or copy the code?
|
Can I get an opinion here on the next steps? or approval :) |
|
Ok, so if there's no objections here :) I'll be merging this soonish. |
|
I think it's okay to land, although we'd need to be cautious if we need to update the package in the future and something has changed(bundling that dep). Now with exclusion from the |
youknowriad
force-pushed
the
add/use-resize-observer
branch
from
7dfe001 to
9398fcf
Compare
|
Ok, so I've just copied the code of the dependency in our package directly to avoid the license conflict. |
There was a problem hiding this comment.
Gave a quick look at the changes on Storybook, everything seems to work as expected.
Could you also add entries to the components and compose packages' CHANGELOG ?
| <iframe | ||
| aria-hidden="true" | ||
| frameborder="0" | ||
| src="about:blank" | ||
| style="display: block; opacity: 0; position: absolute; top: 0px; left: 0px; height: 100%; width: 100%; overflow: hidden; pointer-events: none; z-index: -1;" | ||
| tabindex="-1" | ||
| /> |
There was a problem hiding this comment.
Should we not expect this div to be rendered here instead of the iframe ?
(same for the Flyout's snapshot)
There was a problem hiding this comment.
yes, I wasn't able to figure out why the snapshots didn't match here. My guess is that it has something to do with act and unit test weirdness. Any ideas?
There was a problem hiding this comment.
Not really. I don't think it's to do with act, because the snapshot is being created from a very simple test — I also tried using Jest's fake timers, but nothing changed.
Maybe this is a symptom of an issue with the current implementation?
There was a problem hiding this comment.
Actually, it seems it's because useResizeObserver from @wordpress/compose is globally mocked for some reason 😄
gutenberg/test/unit/config/global-mocks.js
Lines 6 to 9 in dc91c15
I think since these snapshots are not necessarily here to test that the resize observer is in DOM, it should be fine to update them here.
I'll also check why this mock is there in the first place.
There was a problem hiding this comment.
Ok I've removed the global mock in my last commit. db77508
There was a problem hiding this comment.
That explains it 😂 Thank for looking more into it!
AFAIK, I didn't change anything in either of these packages, it's just the implementation details that changed (so far). What exactly should I be mentioning in the changelog? |
We switched from an external library ( |
youknowriad
force-pushed
the
add/use-resize-observer
branch
from
db77508 to
0eeaa5b
Compare
There was a problem hiding this comment.
Gave it another round of tests, and components seem to work as expected 🚀
Not sure if we want to add a bunch of unit tests for the web counterpart (it looks like there are only some for the native side), but otherwise LGTM
Thank you for working on this, @youknowriad !
|
I tried adding tests but for some reason, I was not able to trigger the size changes. I wonder if there's something going on with jsdom. Anyway, I'd appreciate someone taking a look :) |
|
Just noticed in https://codehealth.vercel.app that this PR improved the "time to first block" (load metric) numbers significantly :) |
Not sure if I understand this PR correctly, but isn't the new |
You have a point — the PR's description does mention:
.. but looking at the code changes, it seems like the new
As Riad is AFK for probably another 7-8 weeks, I'm not sure about the reason for the mismatch between the PR description and the actual outcome, and I don't know of any further plans regarding this piece of functionality. We could look into exporting What to other folks involved in this PR think? |
mburridge
added
the
Needs Dev Note
|
Added label in case this needs a dev note for 6.1 release. |
|
cc @youknowriad |
|
Yes, this was always meant to be a first step towards a new API that would allow us to remove a number of useless divs and containers in several components. I've actually started the follow-up a long time ago #41001 just didn't have time to get back to it yet. And for this particular PR and I don't think a dev note is needed as it doesn't change the API at all. |
mburridge
removed
the
Needs Dev Note
|
Thanks @youknowriad, I've removed the label. |
What?
This is a complementary to #40505 It replaces react-resize-aware with use-resize-observer.
Why?
The reason for this is that use-resize-observer can be used directly as a "ref" meaning we can inject it inside useBlockProps if needed or we can pass it a ref directly while
react-resize-awareforces us to render an extra element in the DOM.Also, we've had our share of bugs in Firefox and such of "patterns" not showing the right height... I wonder if this change will solve these issues (though hard to reproduce anyway)
How?
Testing Instructions
That hook is used in several places, the most notable ones are "pattern previews" or block previews, so we can check that patterns are still showing as intended.