Use WeakMap for component tracking #100

helixbass · 2018-03-04T02:02:25Z

Fixes #97. Replaces #95

In this PR:

Use WeakMap (or polyfilled version) rather than static properties to keep track of whether isCompactable()/isMapperComponent()

codecov · 2018-03-04T02:04:55Z

Codecov Report

Merging #100 into master will decrease coverage by 0.13%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
- Coverage   96.45%   96.32%   -0.14%     
==========================================
  Files          65       66       +1     
  Lines         480      490      +10     
==========================================
+ Hits          463      472       +9     
- Misses         17       18       +1

Impacted Files	Coverage Δ
src/utils/createHOCFromMapper.js	`100% <100%> (ø)`	⬆️
src/utils/createCompactableHOC.js	`100% <100%> (ø)`	⬆️
src/utils/WeakMap.js	`66.66% <66.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 748cb3a...4d39016. Read the comment docs.

helixbass · 2018-03-04T02:06:29Z

src/utils/WeakMap.js

@@ -0,0 +1,3 @@
+import WeakMap from 'es6-weak-map'


I used es6-weak-map to polyfill WeakMap. If it's undesirable to add this dependency, I guess we could manually implement a fallback to using a list here - I didn't try to dive deep into the react-proxy discussions re polyfills/fallbacks, but es6-weak-map appears to have O(1) lookup time (rather than O(n) for a list)

Yes but it seems that this polyfill is setting a property on the weakly referenced object (in our case, the enhanced React component) so it would defeat the purpose of this PR.

I'm afraid we'll have to stick to using a list unfortunately.

If we wanted to have O(1)-ish lookup time, we could bucketize this list by taking advantage of the fact that the weakly referenced objects are components and that they (may) have a displayName or name property that we could use as bucket names.

This somehow feels a bit "clumsy" to me though. I am starting to wonder if the better way would not simply be to disable the compactable behavior of components in environments where WeakMap is not supported (that is, IE < 11).

How do you feel about this?

Pinging @neoziro as he may have an opinion on this.

@matthieuprat ah ok I thought that perhaps it was safe (hidden from hoist-non-react-statics) because the referenced implementation describes it as

In a nutshell, the WeakMap shim emulates a WeakMap by adding a hidden property to the key that associates the weak map with the retained object. The shim overrides the ECMAScript 5 methods to cover its tracks.

But indeed, if I change the import from es6-weak-map to es6-weak-map/polyfill (which always uses the polyfill), tests fail

So for now removed es6-weak-map and implemented your suggestion of disabling compactable behavior (by making our utils/WeakMap not track anything if WeakMap does not exist) - not sure how to test the fallback behavior, if I change export default WeakMap || NonTrackingDummyWeakMap to export default NonTrackingDummyWeakMap, it is just "should be merged" tests (and debug test) that fail

I'm not familiar enough with recompact internals to know if disabling compactable behavior when WeakMap is missing is reasonable, but seems like it would make sense if that means that it will behave more like recompose (separate HOCs in the component hierarchy for each recompact HOC used)?

I think it is reasonable. It will indeed behave more like recompose.

One way of testing this behavior would be to create another test file and override the WeakMap global before requireing HOCs that directly or indirectly depend on src/utils/WeakMap:

delete global.WeakMap const { withProps } = require('..') test('NonTrackingDummyWeakMap fallback', () => { /* ... */ })

helixbass · 2018-03-04T02:09:11Z

src/utils/createHOCFromMapper.js

 const createComponentFromMappers = (mappers, childFactory) => {
  const { observablesKey: OBSERVABLES } = getConfig()
  const CONTEXT_TYPES = { [OBSERVABLES]: observablePropType }

-  return class extends Component {
+  const Component = class extends React.Component {
    static [MAPPERS_INFO] = { mappers, childFactory }


I tried following same pattern as in createCompactableHOC (which is what react-proxy does) to get rid of MAPPERS_INFO and store { mappers, childFactory } as the value in allMapperComponents, but was running into errors - don't see the harm in still accessing via MAPPERS_INFO as long as we aren't using its presence to determine whether isMapperComponent()

I think you're right.

helixbass · 2018-03-04T02:10:25Z

src/utils/createCompactableHOC.js

 const isCompactable = Component =>
-  typeof Component === 'function' && Component[compactable]
+  typeof Component === 'function' && allCompactableComponents.has(Component)


Don't know if it makes sense to get rid of typeof Component === 'function' (wasn't sure what it was guarding against/optimizing previously)?

IMO, it was defensive programming (so that isCompactable(null) would return false instead of throwing a TypeError for instance).

I think we can probably get rid of it now that we're using a WeakMap.

@matthieuprat ok removed typeof check here and in isMapperComponent()

helixbass · 2018-03-04T02:11:35Z

src/__tests__/withProps.test.js

@@ -39,4 +39,31 @@ describe('withProps', () => {
    )
    expect(wrapper.equals(<div />)).toBeTruthy()
  })
+
+  it("doesn't compact surrounded HOC that hoists statics", () => {


This tests isCompactable() (test fails against master version of utils/createCompactableHOC)

helixbass · 2018-03-04T02:12:17Z

src/__tests__/withProps.test.js

+    expect(wrapper.prop('foo')).toBe('bar')
+  })
+
+  it("doesn't compact double-surrounded HOC that hoists statics", () => {


This tests isMapperComponent() (test fails against master version of utils/createHOCFromMapper

matthieuprat · 2018-03-05T07:08:59Z

src/utils/WeakMap.js

@@ -0,0 +1,9 @@
+class NonTrackingDummyWeakMap {
+  get() {}
+  set() {}


Might be worth retuning this so that our NonTrackingDummyWeakMap behaves like WeakMap with regard to chaining.

@matthieuprat ok updated

matthieuprat · 2018-03-05T07:09:55Z

src/utils/WeakMap.js

+  }
+}
+
+export default WeakMap || NonTrackingDummyWeakMap


I think this would throw a ReferenceError if WeakMap is not defined.

Maybe we could go with something like the following:

export default (typeof WeakMap === 'undefined' ? NonTrackingDummyWeakMap : WeakMap)

@matthieuprat ok updated

matthieuprat · 2018-03-05T07:12:18Z

You can add /* eslint-disable class-methods-use-this */ to src/utils/WeakMap.js to fix the build.

matthieuprat · 2018-03-05T07:14:07Z

Also, you may want to run yarn test locally before pushing to be fairly confident that both tests and ESLint checks will pass on CI.

helixbass · 2018-03-06T04:32:03Z

src/utils/WeakMap.js

+  }
+}
+
+export default WeakMap || NonTrackingDummyWeakMap


@matthieuprat ok updated

helixbass · 2018-03-06T04:32:22Z

src/utils/WeakMap.js

@@ -0,0 +1,9 @@
+class NonTrackingDummyWeakMap {
+  get() {}
+  set() {}


@matthieuprat ok updated

helixbass · 2018-03-06T04:36:06Z

src/utils/WeakMap.js

@@ -0,0 +1,16 @@
+/* eslint-disable class-methods-use-this */


@matthieuprat sorry for build failure, yarn test was passing, now I'm running yarn lint && yarn test

helixbass · 2018-03-06T04:38:22Z

src/__tests__/utils/WeakMap.test.js

+import React from 'react'
+import { shallow } from 'enzyme'
+
+describe('withProps using NonTrackingDummyWeakMap fallback', () => {


@matthieuprat added tests for fallback NonTrackingDummyWeakMap behavior

matthieuprat · 2018-03-12T20:20:12Z

Thank you! 🎉

helixbass added 3 commits March 3, 2018 19:48

track using WeakMap

c3053cd

working test

53e9c6e

test mapper too

fae8284

helixbass commented Mar 4, 2018

View reviewed changes

helixbass mentioned this pull request Mar 4, 2018

Export symbols (for hoist blacklisting) #95

Closed

fixes from code review

5f0a10c

matthieuprat reviewed Mar 5, 2018

View reviewed changes

test fallback behavior

4d39016

helixbass commented Mar 6, 2018

View reviewed changes

matthieuprat merged commit 109c56e into gregberge:master Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use WeakMap for component tracking #100

Use WeakMap for component tracking #100

helixbass commented Mar 4, 2018

codecov bot commented Mar 4, 2018 •

edited

Loading

helixbass Mar 4, 2018

matthieuprat Mar 4, 2018

matthieuprat Mar 4, 2018

helixbass Mar 4, 2018

matthieuprat Mar 5, 2018

matthieuprat Mar 5, 2018

helixbass Mar 4, 2018

matthieuprat Mar 4, 2018

helixbass Mar 4, 2018

matthieuprat Mar 4, 2018

helixbass Mar 4, 2018

helixbass Mar 4, 2018

helixbass Mar 4, 2018

matthieuprat Mar 5, 2018

helixbass Mar 6, 2018

matthieuprat Mar 5, 2018

helixbass Mar 6, 2018

matthieuprat commented Mar 5, 2018

matthieuprat commented Mar 5, 2018

helixbass Mar 6, 2018

helixbass Mar 6, 2018

helixbass Mar 6, 2018

helixbass Mar 6, 2018

matthieuprat commented Mar 12, 2018

Use WeakMap for component tracking #100

Use WeakMap for component tracking #100

Conversation

helixbass commented Mar 4, 2018

codecov bot commented Mar 4, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieuprat commented Mar 5, 2018

matthieuprat commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieuprat commented Mar 12, 2018

codecov bot commented Mar 4, 2018 •

edited

Loading