remove dependeny on resize-observer-polyfill in ParentSize #254
Conversation
sto3psl
changed the title
|
Thanks for the PR @sto3psl! Definitely agree this is the happy path in the future. Will land this in an upcoming release. |
| import ResizeObserver from 'resize-observer-polyfill'; | ||
|
|
||
| if (!global.ResizeObserver) { | ||
| global.ResizeObserver = ResizeObserver; |
There was a problem hiding this comment.
should this be window && !window.ResizeObserver instead of global?
| @@ -54,6 +54,8 @@ let chartToRender = withParentSize(MySuperCoolVxChart); | |||
|
|
|||
| You might do the same thing using the `ParentSize` component. | |||
|
|
|||
| *Note: `ParentSize` uses [ResizeObserver](https://developers.google.com/web/updates/2016/10/resizeobserver) which is currently only supported in Chrome 64 and newer ([caniuse](https://caniuse.com/#feat=resizeobserver)). If you need support for other browsers you need a polyfill like `resize-observer-polyfill` which is only a `npm install -S resize-observer-polyfill` away.* | |||
|
|
|||
| this.setTarget = this.setTarget.bind(this); | ||
| } | ||
| componentDidMount() { | ||
| this.ro = new ResizeObserver((entries, observer) => { |
There was a problem hiding this comment.
We should probably add an existence check here and thow an error saying to include the polyfill to fail early and so folks know why it's not working.
|
I added a
|
Update package.json add error message
|
Hi @sto3psl sorry for the delay on this! I wanted to chat with some folks about approaches for handling polyfills in libraries. The nice part of So then it comes a question of bundle sizes and library responsibility. It's true that out of the box you'll be including more js then needed if the polyfill is unnecessary. Though, this can be solved at the bundle tool level. You should be able to alias out the polyfill so it's not included in the bundle. My current thinking is to continue including the polyfill for ease of use, not having to respond to issues about xyzResizeObserver polyfills, and leaning to letting app developers decided how to slim down the bundle size. It's a tradeoff i know, but I see value in ease of use and correctness by default over extra js. Would love to hear your thoughts. |
|
I don't have strong opinions about that. It's just that I'm used to libraries shipping without polyfills and I think we ship promise and WeakMap polyfills because of some libraries (webpack if I remember correctly). Since we use at least promises anyways we would have that polyfill anyways. I think a good tradeoff here that can be made is to decide based on browser support. Let's say 50% of the browsers support the feature, drop the polyfill, otherwise include it in the library. Since only Chrome currently supports it and people using Anyway, thanks for the great library ✌️ |
|
I think the best approach is to do include polyfills, but provide info and opt-out instructions. Most of the users will be happy about it and not even know or care. |
I'd rather have instructions on how to include the polyfill in case I really need it. Removing code from a library should not be the concern of the consumer who, in most cases, doesn't have control of the libraries code. The idea of changing my bundlers config to exclude code from a third-party library doesn't speak well to me.
I fully agree on the easier quick start for newcomers and maybe we can also teach them about the use of polyfills, by providing nice errors for them, if something goes wrong. Giving easy understandable warnings and information on how to solve the problem is, in my opinion, more helpful than not confronting them with the problem at all. In this specific case with |
|
Thinking about it, I actually agree. |
|
Going to close this now and maybe come back if 1 more browser supports |
Motivation
To keep bundles slim and give the user of this library a little bit more control I suggest removing the dependency of
resize-observer-polyfillfromParentSize.There is a lot of discussion if library authors should ship with polyfills included or not, that's why this is just a suggestion that would make my team at work switch even faster to vx 😄. I think it's fine to tell the user of the library to include a polyfill if needed to save bytes. I'm happy to discuss this!
💥 Breaking Changes
ParentSizedoesn't useresize-observer-polyfillanymore.📝 Documentation
ParentSizereadme.Thank you for this library btw. I'm currently convincing my team to switch to vx for our visualizations and it is great to use, whether or not this change happens.