Skip to content
This repository has been archived by the owner. It is now read-only.

Add type inference RFC #16

Merged
merged 2 commits into from Apr 9, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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.


# Summary

Flow v0.85 changed the way generic type inference works in a way which meant we were forced to explicitly provide type annotations for `createPlugin`.
After further analysis, I believe we can update the our type definitions slightly in a non-breaking way which will add suppot for type inference when
defining plugins.

# Motivation

One of the biggest strengths of `Fusion.js` is type safe dependency injection. Prior to flow v0.85, this was possible without any explicit type annotations.

For example, prior to v0.85:

```js
export default createPlugin({
deps: {
logger: LoggerToken,
},
provides({logger}) {
return 5;
}
});
```

After flow v0.85

```js
type DepsType = {
logger: typeof LoggerToken,
}
type ProvidesType = number;
export default createPlugin<DepsType, ProvidesType>({
deps: {
logger: LoggerToken,
},
provides({logger}) {
return 5;
}
});
```

The additional requirement of declaring the dependency types and API type explicitly causes some unnecessary friction when defining plugins which could be improved
with the ability to infer these types.

# Detailed Design

While type inference of the `createPlugin` API does not work, some generic type inference across file boundaries does. For example:

```js
export function identity<T>(a: T): T {
return a;
}
// str is correctly inferred as a string
export const str = identity('str');
```

So why does this example of generic type inference work but our `createPlugin` API does not? In summary, "input positions reachable from exports must be annotated."
This is described in detail in this [article](https://medium.com/flow-type/asking-for-required-annotations-64d4f9c1edf8).

So in order to fix type inference in our `createPlugin` API, we need to find which of the generics are used in input positions that are reachable by exports.
Looking at a simplified version of our `createPlugin` API:

```js
type FusionPlugin<Deps, Service> = {|
deps?: Deps,
provides?: (Deps: $ObjMap<Deps & {}, ExtractReturnType>) => Service,
middleware?: (
Deps: $ObjMap<Deps & {}, ExtractReturnType>,
Service: Service
) => Middleware,
cleanup?: (service: Service) => Promise<void>,
|};
function createPlugin<Deps, Service>(p: FusionPlugin<Deps, Service>): FusionPlugin<Deps, Service> {
// ...
}
```

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

the following example:

```js
export const plugin = createPlugin({
provides: () => {
return 5;
}
cleanup(service) => {
console.log(service);
}
});
// other file
import {plugin} from 'file';
plugin.cleanup('test');
```

In this case, the `service` argument to `cleanup` is inferred as `number | string` while with fusion semantics it should be `number` and result in an error
when called with a string. This does not match the flow semantics however due to the process of subtyping ([read more](https://flow.org/en/docs/lang/subtypes/)).

Taking a step back, if we look at our `createPlugin` API we can take advantage of the fact that the return value from `createPlugin` is designed to be opaque.
In other words, it is not designed to have its properties accessed directly by consumers like in the cleanup example.

There are two parts of the type checking of fusion plugins. The first part is in the `createPlugin` call itself. This makes sure all the parts of the plugin are
consistent. For example, the declared deps match the types of the deps passed into `provides` and `middleware`, and the type returned from `provides` matches the
type injected into `cleanup` and `middleware`. All of this can be done without having any return type from `createPlugin`.

The second part is type checking the `app.register` and `app.enhance` calls. This is done to verify the type of the token matches the provides type of the plugin,
and this is the part which depends on the return type of `createPlugin`. The key thing to notice here is that we only need the type of the `provides` method of
the plugin to verify it matches the token. At this point, deps, cleanup, and middleware are all irrelevant.

Because `createPlugin` should return an opaque type and because we only need the `provides` type to verify with `app.register`, we can update the `createPlugin`
return type to remove all generics in input positions without losing any type safety for consumers. There are several ways we could do this, but the simplest
would be replacing the `service` generic with `any` in the return type of `createPlugin` whenever it is referenced in an input position. We also need to specify
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

+deps?: Deps,
+provides?: (Deps: $ObjMap<Deps & {}, ExtractReturnType>) => Service,
+middleware?: (
Deps: $ObjMap<Deps & {}, ExtractReturnType>,
Service: Service
) => Middleware,
+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.

+deps?: Deps,
+provides?: (Deps: $ObjMap<Deps & {}, ExtractReturnType>) => Service,
+middleware?: (
Deps: $ObjMap<Deps & {}, ExtractReturnType>,
Service: any
) => Middleware,
+cleanup?: (service: any) => Promise<void>,
|};
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
}
```

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.


While there is type information lost, I would argue it is not type information that should be used in any meaningful way as it is not recommended
to directly access properties of fusion plugins.

The only other drawback we could potentially see is if future changes to flow or the fusion plugin API cause us to lose the ability to get type inference
we will need to migrate all usages of `createPlugin` to include explicit type annotations, as we have done in the past.

---

# 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.

would still exist on the object.

Another alternative is we can use an identity function type along with `$Call` to mimic the behavior of `$ObjMap`. While this technically works, the flow team
has said this is a bug and is not guaranteed to continue working in future releases. (However the $ObjMap is behaving correctly).
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.