Skip to content
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

Add type inference RFC #16

Merged
merged 2 commits into from Apr 9, 2019

Conversation

Projects
None yet
6 participants
@ganemone
Copy link
Contributor

commented Mar 29, 2019

No description provided.

@fusion-bot fusion-bot bot added the docs label Mar 29, 2019

the properties as covariant.

```js
type CreatePluginArgs<+Deps, +Service> = {|

This comment has been minimized.

Copy link
@AlexMSmithCA

AlexMSmithCA Mar 29, 2019

Member

Does this work? I think the generic declaration for Service here might need to be invariant as it is still being passed in as an input.

This comment has been minimized.

Copy link
@ganemone

ganemone Mar 29, 2019

Author Contributor

Yea you are correct - copy paste error

This will allow us to get type inference with fusion plugins across module boundaries while still getting all the performance benefits of the types first
flow architecture and keeping all the same type safety for fusion consumers.

# Drawbacks

This comment has been minimized.

Copy link
@AlexMSmithCA

AlexMSmithCA Mar 29, 2019

Member

We make the assumption that that all plugins are created using createPlugin. By relaxing the types for FusionPlugin, folks not using createPlugin may generate objects that may satisfy this new type but we would consider invalid.

Technically a regression, as these are caught today, but would not be caught here.

This comment has been minimized.

Copy link
@ganemone

ganemone Mar 29, 2019

Author Contributor

Realistically I don't think we need to worry about this. We could easily catch this at runtime by either using a class constructor in the createPlugin method or attaching a Symbol that is not exported.

# Alternatives

There are various alternatives to how we can hack the type system to remove references to generics in input positions. One possibility is we could completely remove
the presence of the `cleanup` and `middleware` properties from the return type of `createPlugin`. I think this is worse than using `any` since those properties

This comment has been minimized.

Copy link
@AlexMSmithCA

AlexMSmithCA Mar 29, 2019

Member

Agreed! This would also require some hackery in fusion-core if we went this route.

@@ -0,0 +1,168 @@
# FusionJS Plugin Type Inference

This comment has been minimized.

Copy link
@AlexMSmithCA

AlexMSmithCA Mar 29, 2019

Member

This is great! Thanks @ganemone for putting this together. My personal opinion here is that the trade-off for a better developer experience is worth the loss of type coverage.

It is unfortunate that Flow in its present state lacks global inference which would solve this problem without having to resort to workarounds like this.

This comment has been minimized.

Copy link
@ganemone

ganemone Mar 29, 2019

Author Contributor

To be fair to flow, it seems they have some good reasons for not supporting global type inference which will help performance in a monorepo, but it would be nice.

+cleanup?: (service: Service) => Promise<void>,
|};
type FusionPlugin<+Deps, +Service> = {|

This comment has been minimized.

Copy link
@AlexMSmithCA

AlexMSmithCA Mar 29, 2019

Member

Worth noting that by declaring all of these properties as covariant, plugins effectively become read-only. We lose the ability to extend existing plugins like we do in fusion-react.

Arguably a drawback as the current implementation supports this behavior.

This comment has been minimized.

Copy link
@ganemone

ganemone Mar 29, 2019

Author Contributor

I think we should be able to do the same thing in an immutable way by returning a new plugin rather than mutating the existing one. However, we can always do mutations like this with //$FlowFixMe as long as the pattern is not done by direct consumers.

|};
function createPlugin<Deps, Service>(p: CreatePluginArgs<Deps, Service>): FusionPlugin<Deps, Service> {
// ...

This comment has been minimized.

Copy link
@angus-c

angus-c Mar 29, 2019

Can you show how the syntax of your original example (the one with LoggerToken dep) will look under this plan?

This comment has been minimized.

Copy link
@ganemone

ganemone Mar 29, 2019

Author Contributor

It will just revert it to the way it used to work before the flow changes. So the prior example here: https://github.com/fusionjs/rfcs/pull/16/files#diff-8b2c91837314197ff15dc6103f719e55R16

This comment has been minimized.

Copy link
@angus-c
the properties as covariant.

```js
type CreatePluginArgs<+Deps, Service> = {|

This comment has been minimized.

Copy link
@albertywu

albertywu Mar 29, 2019

It's not clear to me why type CreatePluginArgs is necessary since it's almost identical to type FusionPlugin below. The following compiles just fine, but maybe I'm missing something? try flow

This comment has been minimized.

Copy link
@ganemone

ganemone Mar 29, 2019

Author Contributor

In your example, you lose type information about service in the cleanup and middleware functions.

This comment has been minimized.

Copy link
@AlexMSmithCA

AlexMSmithCA Mar 29, 2019

Member

I haven't played around with it, but we could possibly dedup these by using object type spread. Arguably not as understandable as the duplication though.

@micburks

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Really nice writeup. Makes perfect sense to me 👍

@KevinGrandon
Copy link

left a comment

I think this makes sense to me, but we might want to get a few more approval stamps if there haven't been any others. Thanks for taking the time to put this together.

In this example, the `Service` argument is in the input position in the `cleanup` function as well as the `middleware` function. Despite `deps` being used
in the `provides` and `middleware` function, it is not directly reachable from exports because of the usage of `$ObjMap`.

Looking at the `cleanup` method is the simplext example. The reason the `Service` generic here qualifies as a type in the input position is demonstrated by

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Apr 8, 2019

nit: typo: simplext

@ganemone

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

!merge

@fusion-bot fusion-bot bot merged commit a23938d into fusionjs:master Apr 9, 2019

7 checks passed

license/cla Contributor License Agreement is signed.
Details
probot/label-docs-pr Docs label has been set (or unset)
probot/migrations Migration guide provided
probot/pr-label At least one required semver-related label exists
probot/pr-title PR title is valid
probot/release-verification Verification not required for this PR.
probot/todos All TODOs have open issues
@fusion-bot

This comment has been minimized.

Copy link

commented Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.