-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Remove fixPolyfills.ts, except when bundling for React Native. #5962
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5962 +/- ##
==========================================
+ Coverage 94.71% 95.11% +0.39%
==========================================
Files 91 90 -1
Lines 3730 3705 -25
Branches 883 911 +28
==========================================
- Hits 3533 3524 -9
+ Misses 174 158 -16
Partials 23 23 Continue to review full report at Codecov.
|
const testMap = new Map(); | ||
if (testMap.set(1, 2) !== testMap) { | ||
const { set } = testMap; | ||
Map.prototype.set = function (...args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These shims are not specific to React Native, but we also do not rely on the return values of Map.prototype.set
or Set.prototype.add
anywhere in the Apollo Client codebase anymore. I'm leaving them here so folks can easily find and copy the code if they need it.
React Native used to have Map and Set polyfills that relied on tagging objects with a special __MAP_POLYFILL_INTERNAL_HASH__ property, which breaks down when the object to be tagged happens to be frozen. In 833072e (#3964) I introduced a workaround to make sure objects got tagged before becoming non-extensible, which robustly solved the problem for any Map and Set polyfills that rely on an object tagging strategy. Note that Apollo Client freezes objects only in development (using maybeDeepFreeze), so the problem fixPolyfills.ts was intended to solve was only ever a problem in development. I also submitted a PR to React Native that make their Map and Set polyfills store non-extensible objects in an array, rather than attempting to tag them: facebook/react-native#21492. Those changes were first released in React Native 0.59.0, so technically the fixPolyfills.ts logic should not be necessary for anyone using that version or later. Since then, React Native has stopped using any polyfills for Map and Set (yay!), so the need for the workaround has been even further reduced: facebook/react-native@93b9ac7 Those changes were first released in React Native 0.61.0, which is still the latest minor version (0.61.5 is latest). I'm not sure how many people are still using older versions of React Native, or what sort of LTS policies they have. Expo SDK 36 uses 0.61.4, for what it's worth: https://docs.expo.io/versions/latest/sdk/overview/ In any case, I think we can eliminate these polyfills from the default bundle, as long as we take some care to include them when bundling for React Native. This strategy uses a combination of techniques for selective bundling in React Native: https://facebook.github.io/react-native/docs/platform-specific-code#platform-specific-extensions facebook/react-native#2208
15005ff
to
07d052b
Compare
@hwillson If we were confident that everyone using React Native should be using at least 0.59.0, then we could remove this logic altogether, and avoid the React Native-specific conditional bundling logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @benjamn - these changes look great!
Regarding being able to drop these changes altogether if people are using >= 0.59.0, that's a great question. 0.59.0 introduced hooks support, so I would imagine a lot of people updated. We could always consider declaring 0.59.0 to be the minimum and drop a note about it in the Changelog
, but these changes don't add that much extra overhead when bundling for React Native (and 0 to the default bundle when not) so we can probably ride things out for a bit longer.
React Native used to have
Map
andSet
polyfills that relied on tagging objects with a special__MAP_POLYFILL_INTERNAL_HASH__
property, which breaks down when the object to be tagged happens to be frozen.In 833072e (#3964) I introduced a workaround to make sure objects got tagged before becoming non-extensible, which robustly solved the problem for any
Map
andSet
polyfills that rely on an object tagging strategy (not just the React Native ones).A while back, I submitted this PR to React Native that make their
Map
andSet
polyfills store non-extensible objects in an array, rather than attempting to tag them. Those changes were first released in React Native 0.59.0, so technically thefixPolyfills.ts
logic should no longer be necessary for anyone using that version or later.Since then, React Native has stopped using any polyfills for
Map
andSet
(yay!), so the need for the workaround has been even further reduced: facebook/react-native@93b9ac7Those changes were first released in React Native 0.61.0, which is still the latest minor version (0.61.5 is latest). I'm not sure how many people are still using older versions of React Native, or what sort of LTS policies they have. Expo SDK 36 uses 0.61.4, for what it's worth.
In any case, I think we can eliminate these polyfills from the default bundle, as long as we take some care to include them when bundling for React Native. This strategy uses a combination of techniques for selective bundling in React Native:
Non-React Native applications that want to support older JavaScript environments should probably use the
core-js
polyfills forMap
andSet
.