[email protected]: hooks, forwardRef support, and better types #83

aem · 2020-04-06T14:32:49Z

This is a proposal for a v4 of general-store. There's not actually any new functionality, but I figure that correct TS typings + the ESM main module is enough to warrant a new major version given that those are both technically breaking changes. Realistically, internally at HubSpot I don't think our repackage will have to bump a major version, since we shim CJS modules anyway I believe the default import of general store will still work, and none of the other code changes are breaking - connect + useStoreDependency still work fine.

TypeScript

In v3, I did the TypeScript conversion, but given the complexity of the types (and the fact that I hadn't tried to use General Store in an actual TS project), the types were compiling successfully but didn't actually type anything in a meaningful way. The old types wouldn't properly validate the props on a connected component to verify they were a superset of the general store dependencies, and the output of a useStoreDependency call was always typed as any.

This PR fully reworks the TS types using generics and inferred conditional types to ensure that all General Store calls are typed properly. The TS compiler will fail the connect call if the wrapped component doesn't include all of the dependencies in the DependencyMap in its props type, and useStoreDependency correctly infers its return type assuming the dependency has an inferrable type (e.g. the TS compiler doesn't have to resort to any).

Type checks in action

Everything type checks just fine!

But if the store is of an unexpected type, connect and useStoreDependency both throw.

Connect also throws if the dependencies don't sufficiently overlap with the component props.

Lastly, we now publish the project's TS types and reference them in the package.json, allowing external projects to pull in the type definitions properly.

Connect

The old legacy version of connect was a bit bloated, and relied on a lot of extra mapping + reducing of objects, creating some internal performance problems. The new version just uses the same internals as useStoreDependency, allowing us to vastly simplify the internal code of General Store. By my estimate (some rough back of the napkin math) this reduces the bundle footprint of General Store by more than 30%.

One small change that was made - we no longer check the length on deref functions to determine whether or not to pass the state/dependency map. Passing extra arguments shouldn't cause a problem for existing dependencies, and the fact that there were actually different code paths based on the number of arguments your deref took always felt weird to me.

Misc

The GeneralStore.ts entry point now uses ES2015 exports rather than CJS. The rest of the module uses ES2015 modules, so by converting the entry point to do the same, the library is now fully ESM compliant and will be treeshake-able.
The docs have been updated to clarify how component composition with connect works - the second state argument was never documented up until now, you'd only find out about it from reading prior art or the library's actual source
Some tests are now obsolete with the new fully-hooks-based implementation. Those tests were removed and not backfilled, but the test suite is still comprehensive.

@colbyr

henryqdineen · 2020-04-06T15:01:06Z

I'm probably not the best to review the TypeScript changes but everything else looks great. 👍

Friss

Couple thoughts inline.

One thought is also if the equals func should care / have a different branch for arrays over regular objects. Since the current for in will just give us the indexes. Or is it only used on objects?

Also the Typescript typing changes are a bit over my head but I saw more types and that is always good right? :badpokerface:

Friss · 2020-04-09T04:35:52Z

src/dependencies/useStoreDependency.ts


  const newValue = calculate(dependency, props);
-  if (!shallowEqual(newValue, dependencyValue)) {
-    setDependencyValue(newValue);
+  if (!deepEqual(newValue, dependencyValue.dependency)) {


Curious why the change here from shallow to deep?

whoops, copypasta from the new version of connect. connect requires the deepEqual because the state in that component is a DependencyMap, so we need to check that the entire map is the same and trigger the setState if any of the sub-objects is meaningfully different, but in useStoreDependency it's a single result so it can be a shallowEqual. I'll update

Friss · 2020-04-09T04:45:42Z

src/utils/ObjectUtils.ts

+          return false;
+        }
+      } else {
+        if (!_equal(obj1[property], obj2[property], false)) {


Nit: could just return this.

quick update, ya can't actually! since this is iterating over every property, returning the value here will short circuit the logic and could return a false-positive if this logic isn't the last property checked

Friss · 2020-04-09T04:47:32Z

src/utils/__tests__/ObjectUtils-test.js

+
+  describe('deepEqual', () => {
+    it('deeply compares values', () => {
+      expect(deepEqual(1, 1)).toBe(true);


nit: Array test?

aem · 2020-04-09T13:58:58Z

Also the Typescript typing changes are a bit over my head but I saw more types and that is always good right? :badpokerface:

I don't know if they're perfect, but they definitely do work, which is all I was really hoping for. ¯\_(ツ)_/¯ I'm excited to see how many CRM things explode when we introduce the TS types to those components. Excited for lots of weird stuff like prop: string | Map | List | null

colbyr

Oh man. I've been having deep nostalgia for GeneralStore the last few weeks. I'm absolutely going to build something with it once you release this 🤩

Passing extra arguments shouldn't cause a problem for existing dependencies, and the fact that there were actually different code paths based on the number of arguments your deref took always felt weird to me.

I think that had to do with the StoreDependencyMixin and like [email protected] when dependencies could read from the state of the component they were bound to. We used the deref arg length, to decide if the dependency needed to re-evaluate when the component state changed.

general-store/src/dependencies/DependencyMap.js

Lines 147 to 157 in 69902c9

    
           export function calculateForStateChange( 
        
             dependencies: DependencyMap, 
        
             props: Object, 
        
             state: ?Object 
        
           ): Object { 
        
             return oFilterMap( 
        
               dependencies, 
        
               (dep) => dep.deref && dep.deref.length > 1, 
        
               (dep) => calculate(dep, props, state) 
        
             ); 
        
           }

At the time it saved a ton of cycles because mostly deps didn't use state, but I don't think that's relevant to the more recent connect strategies! It's probably safe to remove that behavior, but y'all would probably know better than me 😉

aem · 2020-04-09T19:59:54Z

I think that had to do with the StoreDependencyMixin and like [email protected] when dependencies could read from the state of the component they were bound to. We used the deref arg length, to decide if the dependency needed to re-evaluate when the component state changed.

Wow, TIL. That makes perfect sense, and definitely explains why the connect/StoreDependencyMixin codepaths were so much more complex than what was required to get useStoreDependency working. Regardless, since everything is props now, seems like unifying the logic and shedding the extra weight of the code needed to support the old architecture is worthwhile.

colbyr · 2020-04-09T20:07:36Z

Wow, TIL. That makes perfect sense, and definitely explains why the connect/StoreDependencyMixin codepaths were so much more complex than what was required to get useStoreDependency working.

Yeah, it was kind of a mess. Good riddance! 😛

aem · 2020-04-13T15:06:43Z

Gonna look at integrating https://github.com/joarwilk/flowgen into the build process. If we're doubling down on shipping types we should probably output Flow types as well for completeness

aem · 2020-05-01T18:54:53Z

So good news, I finally got flow typegen working. Bad news, I'm not sure the types will be super useful 😞 since the TS typings require conditional types to work well and Flow doesn't support conditional types I'm not sure how useful they'll be. At least there's something though?

aem · 2020-05-01T18:58:38Z

Also, I think I'm going to just submit this as one huge PR. I tried to split things out atomically, but the types for useStoreDependency and the old iteration of connect don't coalesce very nicely, so without the connect refactor the types don't make sense, but without the type refactor the connect refactor doesn't work well either. Basically, I accidentally created a chicken + egg problem for myself and it'll be hard to untangle that

aem · 2020-05-07T16:44:56Z

@colbyr @Friss @henryqdineen going to merge and publish under general-store@next and leaving npm latest at 3.2 for a bit. @Friss has volunteered to help test it out in a nontrivial app this weekend since the connect tests are a bit contrived

aem · 2020-05-07T16:51:29Z

$ npm info general-store@latest

[email protected] | MIT | deps: 1 | versions: 45
Simple, flexible store implementation for Flux.
https://github.com/HubSpot/general-store

keywords: flux, store, react

dist
.tarball: https://registry.npmjs.org/general-store/-/general-store-3.2.1.tgz
.shasum: 9a94e29204d7f3321044f4b38e572fed85dce269
.integrity: sha512-Mvgo3f/e/6Z+nSpNHWwqEE+hHEdE2nNwsOdKPTxT4JXqso7oUINjoFpkaPHCvjSotXarTFxDh9VuNpnPm6LVkQ==
.unpackedSize: 208.7 kB

dependencies:
hoist-non-react-statics: 3.3.0 

maintainers:
- colbyr <[email protected]>

dist-tags:
latest: 3.2.1  next: 4.0.0    

published 5 months ago by amarkon <[email protected]>

4.0.0 published under the next tag, will be published as latest once I get confirmation things seem to be working

Friss · 2020-05-11T00:16:42Z

4.0 seems to be working well. I did have to change from import GeneralStore from 'general-store' to import {define} from 'general-store' (which was/is expected) My app is made up of a few define and defineFactory stores that all use the connect HOC.

We should also update the docs to reflect that you would need to do a import * as GeneralStore or do import {define} for the few examples inline.

aem · 2020-05-11T02:05:58Z

good call, I'll update the docs before publishing latest. probably not worth building automation for this internally since it's an easy find+replace?

Friss · 2020-05-11T02:25:36Z

Might take a bit of coordination to migrate libraries and UIs with this change but the underlying change is pretty simple.

aem · 2020-05-13T17:05:38Z

4.0.1 was just promoted to latest on npm and includes the performance regression fix from #84 on top of the changes in this PR

aem added 3 commits April 1, 2020 22:01

refactor TS types to make useStoreDependency type correctly

62760d6

rewrite connect to use hooks, forwardRef, clean up types

43e257c

clean up docs, remove unused deps

0a6a7dd

aem requested review from henryqdineen and Friss April 6, 2020 14:32

aem added 2 commits April 6, 2020 10:52

fix typo + clarity

d5d0104

remove unused esm field

10fbe9d

aem mentioned this pull request Apr 6, 2020

React 16 Updates #62

Open

Friss reviewed Apr 9, 2020

View reviewed changes

colbyr approved these changes Apr 9, 2020

View reviewed changes

aem added 3 commits May 1, 2020 12:58

some code review comments

53ad569

cant early exit here

3c5aae0

generate flow types and bundle TS types

cc2b6b8

aem marked this pull request as ready for review May 1, 2020 18:58

export some types for generation and update typegen

3f8f7a4

aem changed the title ~~RFC: [email protected]~~ [email protected]: hooks, forwardRef support, and better types May 1, 2020

aem merged commit 913f9f5 into master May 7, 2020

aem deleted the adam/ts-types-connect-refactor branch May 7, 2020 16:45

aem mentioned this pull request May 11, 2020

Update docs to reflect ESM #85

Merged

This was referenced May 13, 2020

Rewrite connect to use useStoreDependency #81

Closed

Publish TS types #82

Closed

Convert to ESModules #80

Closed

handle focus when BaseComponent is an object #66

Closed

Support arrow function components #67

Closed

gmcnaughton mentioned this pull request Nov 1, 2021

Fix error connecting arrow function components #106

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[email protected]: hooks, forwardRef support, and better types #83

[email protected]: hooks, forwardRef support, and better types #83

aem commented Apr 6, 2020 •

edited

Loading

henryqdineen commented Apr 6, 2020

Friss left a comment

Friss Apr 9, 2020

aem Apr 9, 2020

Friss Apr 9, 2020

aem Apr 9, 2020

aem May 7, 2020

Friss Apr 9, 2020

aem commented Apr 9, 2020

colbyr left a comment •

edited

Loading

aem commented Apr 9, 2020

colbyr commented Apr 9, 2020

aem commented Apr 13, 2020

aem commented May 1, 2020

aem commented May 1, 2020

aem commented May 7, 2020

aem commented May 7, 2020

Friss commented May 11, 2020

aem commented May 11, 2020

Friss commented May 11, 2020

aem commented May 13, 2020

	export function calculateForStateChange(
	dependencies: DependencyMap,
	props: Object,
	state: ?Object
	): Object {
	return oFilterMap(
	dependencies,
	(dep) => dep.deref && dep.deref.length > 1,
	(dep) => calculate(dep, props, state)
	);
	}

[email protected]: hooks, forwardRef support, and better types #83

[email protected]: hooks, forwardRef support, and better types #83

Conversation

aem commented Apr 6, 2020 • edited Loading

TypeScript

Connect

Misc

henryqdineen commented Apr 6, 2020

Friss left a comment

Choose a reason for hiding this comment

Friss Apr 9, 2020

Choose a reason for hiding this comment

aem Apr 9, 2020

Choose a reason for hiding this comment

Friss Apr 9, 2020

Choose a reason for hiding this comment

aem Apr 9, 2020

Choose a reason for hiding this comment

aem May 7, 2020

Choose a reason for hiding this comment

Friss Apr 9, 2020

Choose a reason for hiding this comment

aem commented Apr 9, 2020

colbyr left a comment • edited Loading

Choose a reason for hiding this comment

aem commented Apr 9, 2020

colbyr commented Apr 9, 2020

aem commented Apr 13, 2020

aem commented May 1, 2020

aem commented May 1, 2020

aem commented May 7, 2020

aem commented May 7, 2020

Friss commented May 11, 2020

aem commented May 11, 2020

Friss commented May 11, 2020

aem commented May 13, 2020

aem commented Apr 6, 2020 •

edited

Loading

colbyr left a comment •

edited

Loading