Remove resize observer polyfill #925
Conversation
|
Hi @koddsson, I hear you on this. This has been a trade-off discussion for a couple years now. Here's the previous discussion: #254 (comment) This is going to continue coming up in the future as we get further away from legacy browser support and modern browser support for ResizeObserver widens. Unfortunately, this is something we would have to land in a One thing that is mentioned in the previous discussion is the idea of alias-ing out the polyfill dependency so it's not including in bundles. This isn't ideal though as it requires more work to do the paved road approach of not needing a polyfill. My preference would be to avoid a v2 and breaking change release and to add new components to edit: this is also an approach we've taken with newer components to avoid including polyfills as seen with @visx/annotation: https://github.com/airbnb/visx/tree/master/packages/visx-annotation#%EF%B8%8F-resizeobserver-dependency |
Would this just be copies of the original? So in this case I'd just clone the |
|
@hshoff do you have thoughts on the name of the component? maybe Regarding implementation, I think // user passes in their own polyfill
import { ResizeObserver } from 'my-favorite-polyfill';
() => <ParentSize polyfill={ResizeObserver}>{...}</ParentSize>
// user polutes the `window` object (current implementation)
import ResizeObserver from 'my-favorite-polyfill';
() => <ParentSize >{...}</ParentSize>
// browser policy supports ResizeObserver already
() => <ParentSize >{...}</ParentSize>
// user does nothing, and browser doesn't support
() => <ParentSize >{...}</ParentSize>
// Error: This browser does not support ResizeObserver out of the box
// see example implementation https://github.com/pmndrs/react-use-measure/blob/master/src/web/index.ts#L47Make sense? |
|
|
This component is a clone of `ParentSize` that doesn't use a `ResizeObserver` polyfill.
koddsson
force-pushed
the
remove-polyfill-from-responsive
branch
from
0a3ecac
to
77beb80
Compare
|
I've pushed some changes and update the PR description. I opted for not accepting a polyfill as a prop since I feel like polyfills should just patch the global object but people might disagree. Let me know what y'all think and I can make changes accordingly. |
This reverts commit 4a77cb6.
Until TypeScript lands https://git.io/Jk9FD which adds `ResizeObserver` types.
Pull Request Test Coverage Report for Build 386875981Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
@koddsson thanks for the changes 🙏 do you want to fix the lint issues or do you mind if I push them to this branch? (trying not to merge something that fails CI) |
Feel free to push to the branch, I can also fix them tomorrow if you'd like. |
|
Thanks @koddsson! Will try to get this out today as |
|
heads up this is out as a |
ResizeObserver.My application doesn't require the polyfill for resize observer since my browser support policy only includes browsers that support
ResizeObserver.In keeping with this projects mantra of, "Keep your bundle sizes down and use only the packages you need.", I'm creating new components that don't include the polyfill. Maybe the old components can be removed in future major versions.