Update flow-bin to 0.91.0 #731
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glasserc
force-pushed
the
greenkeeper/flow-bin-0.91.0
branch
from
780c013
to
3222349
Compare
This has to be an exact object and corresponds to what is returned by mapStateToProps. We also have to export the `Props` type, which is the type of the "connected" props, with state and dispatch functions connected up by the container. The functions in `Props` correspond to action dispatchers (a.k.a. "action creators"). Previously, we explicitly named the actions that the component could dispatch, which is kind of cool. Let's keep that, but explicitly re-use the notification actions from the `actions/Notifications.js`, instead of retyping their type signature. Previously, these actions were typed as returning void, but actually they return the same action that's being dispatched. We ignore the returned action so just use "*" where needed so that it will typecheck.
- It helps flow figure out what's going on if `mapStateToProps` has an explicit return type. - `DispatchAPI` is generic (takes "any action"). Express this using "*". This introduces a little bit of unsafety because we could ask it to dispatch non-action things, but fixing this requires exporting a valid type from `actions/notifications.js`, which is a bear because of facebook/flow#7377. - `connect` is now generic and takes `Props`, `OwnProps`, `StateProps`, `DispatchProps`, and two other things I never bothered to look up. However, Flow is smart enough to infer most of these when you put `_`. Unfortunately, `eslint` gets upset because `_` is undefined, so disable linting on this line. To elaborate on the type parameters to `connect`, - `Props` is the set of props actually used by the "raw" component. It is an amalgamation of some of the other type parameters given here. - `OwnProps` is the set of props that are passed to the connected component. In this particular case, we don't pass any props to the component, so pass an exact empty object. - `StateProps` is the set of props extracted using `mapStateToProps`. - `DispatchProps` is a set of "action creators" created using `mapDispatchToProps`. This should also be an "exact object type", and it happens that the `actions/notifications.js` module defines one. It happens that the "dispatching" action creators return the same actions as the functions from that module, so we can just re-use the type.
This one is a bit trickier than Notifications because it has OwnProps (from where it's called in routes.js). The OwnProps must be an exact object, like StateProps and DispatchProps, so we can't pass extra props. The props given by Route includes match, location, and history, none of which are used by App.
This actually caught a bug -- capabilities was being used without being passed.
In order to make the typings line up, I re-used the signature for deleteRecord. Unfortunately, we seem not to always have an `rid` present, so special case that. We'll have to revisit. Additionally, not all calls to `deleteRecord` pass a `last_modified`, so make it optional. In the underlying kinto-http layer, it is, but it shouldn't be being called in situations where it's missing, so we'll have to revisit this too. I also understood that flow has some confusing behaviors with intersections of object types, so just convert stuff to exact objects and combine them using spread. I'm not really sure why this works but it seems to. This also corrects a bug in the typings that isn't actually a real bug -- deleteRecord and redirectTo are missing from the props passed to the component (although they're actually being passed).
These are all linked by the HistoryTable, so do them all at once. This fixes some typing errors: - For some reason, many of these components said they needed the bucket, but didn't actually use it. - For some reason, these containers all passed an updatePath function, though none of them needed it.
This affects both the BucketCollections and CollectionRecords components. We already committed the CollectionRecords component, so just include BucketCollections here. While we're here, clean up the typings of the BucketCollectionsPage container, which needs much fewer actions than it passes.
These are internal and not connected, so their actions can generally have just about any type signature. In particular, many of the forms only *sometimes* call the associated action, waiting first for a confirm().
The data we provide to "update" functions includes at least one of data and permissions but not necessarily both. This makes it unlike the data you get back for those resources. Formalize this in the types of some actions and use them to fix the Attributes and Permissions components that use them. While we're here, tighten up some of the Record containers types (passing more actions than we need again).
While we're here, tighten up a few more types -- sometimes Capabilities is passed but not used, and sometimes updatePath is passed but not needed.
In order to get the StateProps to line up, I had to correct an error where the history field is declared to be both an array of strings and an array of history entries. I'm not a Redux expert but it seems like the history field is managed by the history reducer, which maintains a state which is an array of history entries, so this means that this probably wasn't a real error. Despite getting errors that the StateProps were wrong, I didn't get any errors complaining that the history reducer state was incompatible with the app state. I suspect the typing isn't perfect yet, which is a little scary, but that's JavaScript.
This module contains wrappers around react-router stuff that tries to extract breadcrumb information and fire route-updated messages. Because react-router is higher-order, it's a bit trickier. The solution this PR pushes towards isn't really complete or the cleanest, but it gets us to a passing build and I'm running out of time to work on this. `routeCreator` is a wrapper around `Route` that is also a connected component, so we have to define its prop types. I don't see any obvious way to get it to automatically infer the props that `Route` uses and the type is not exported so we redefine it here, more or less. `Route` does not take `title` so be careful not to pass it in the props that go to `Route`. `Route` is a higher-order-component that wraps our component (using `render`). Our components are all connected components so they don't get any particular props, but they all get the React Router props (match, location, and history) which are defined in `ContextRouter`. We also insert a `ComponentWrapper` which wraps our component. This is what we actually give to `Route`. It gets all the same router props plus a component to call and the connected onRouteUpdated function.
However, let's still try not to pass `routeUpdated` to the component.
glasserc
force-pushed
the
greenkeeper/flow-bin-0.91.0
branch
from
a6b69a5
to
2c8139c
Compare
|
OK, so I think this actually typechecks. I cheated a little bit at the end -- check |
|
Also, some helpful references:
I also understand redux a lot better now.. |
This was referenced Jan 31, 2019
leplatrem
approved these changes
Feb 1, 2019
There was a problem hiding this comment.
Amazing 😮
I read the whole story (was gonna say saga) in the individual commits and everything makes sense.
Just the connect() functions look a bit weird, but bearable with your commit comments (I don't know if it makes sense to copy them into the code base...)
Congrats on the amount of work done here. Let's pray for a v1.0 of flow soon...
| // though `Route` always passes the complete set of router | ||
| // props. We can't reproduce this behavior in correctly-typed | ||
| // code, so let's not even try. | ||
|
|
| }; | ||
| } | ||
|
|
||
| function mapDispatchToProps(dispatch: Dispatch): ActionCreatorOrObjectOfACs { | ||
| // FIXME: component doesn't need NotificationsActions |
There was a problem hiding this comment.
Oops, I thought I had cleaned these all up. If you look at the RecordCreate component, its Props indicates that it only needs createRecord, not any of the other stuff it's being connected to. I'll fix this in another commit.
| return bindActionCreators(NotificationsActions, dispatch); | ||
| } | ||
|
|
||
| export default connect( | ||
| export default connect<Props, {||}, _, _, _, _>( // eslint-disable-line |
There was a problem hiding this comment.
Is the eslint thing something we should track to remove them once eslint will support the new flow-bin?
There was a problem hiding this comment.
I guess so. I opened gajus/eslint-plugin-flowtype#384.
|
I'm not sure if it's worth adding code comments to the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Go big or go home.
Replaces #699.