search

dojo / framework

Dojo Framework. A Progressive Framework for Modern Web Apps
https://dojo.io/
Other
585 stars 78 forks source link

The ResizeObserver shim should not add to `global` #237

Closed dasa closed 5 years ago

dasa commented 5 years ago

Bug

Package Version: 5.0

Code

https://github.com/dojo/framework/blob/master/src/shim/ResizeObserver.ts#L34-L38

Expected behavior:

It returns the ponyfill: https://github.com/que-etc/resize-observer-polyfill#usage-example

Actual behavior:

It sets the ponyfill on global and then returns that. This also doesn't work if using an AMD loader that uses the non-ES6 module since default is then undefined.

matt-gadd commented 5 years ago

The expected behaviour is to be on the global for shim and will be going forward as there are not ponyfills for everything out there externally. See PointerEvents, IntersectionObserver, fetch, Promise, Symbol, WebAnimation and maybe more that now align the same behaviour. Being on the global also allows us to completely elide the entire module during the build.

It looks like we need to update the readme in shim to clarify this, as it currently contradicts the above statement (other than for Promise and Symbol). We will likely be renaming shim to platform in 6.0.0 also.

We would definitely love to see a fix for the AMD bug though if that is an issue.

dasa commented 5 years ago

We would definitely love to see a fix for the AMD bug though if that is an issue.

OK, I can try to create a PR for this. The easy fix would be to enable esModuleInterop in the tsconfig, but that requires TS 2.7. Would you prefer a check inside the shim itself?

matt-gadd commented 5 years ago

ha yeah we're in a bit of catch 22 with esModuleInterop, when we do move upwards from 2.6 we will have that on by default but at the moment I think we have no option other than to check for it in shim? cc @agubler?

dasa commented 5 years ago

when we do move upwards from 2.6 we will have that on by default

Is that soon? I might just wait for that.

matt-gadd commented 5 years ago

@dasa we've been keen to try and support as far back as our initial TS version while we reasonably can. Perhaps switching from:

import Resize from 'resize-observer-polyfill';

to

import * as Resize from 'resize-observer-polyfill';

in the shim will work for now?

dasa commented 5 years ago

The expected behaviour is to be on the global for shim

Does this include array, string, Map, etc?

matt-gadd commented 5 years ago

At the moment no, we only aligned the newest external polyfills/ponyfills to be consistent (Promise and Symbol were the only existing ones that were on the global).

The array and string implementations are more problematic because they do not have the same api shape due to not being on the primitives prototype.

We do have plans in general to try and make this more consistent on the road to v6. We could do with being more thorough on stating the end goal for shim/platform in documentation so apologies on the darkness there!

dasa commented 5 years ago

Would you prefer a check inside the shim itself?

PR created