Skip to content

Improve plugin static typing docs #97

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve plugin static typing docs #97

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 86 additions & 2 deletions documentation/docs/guides/typing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,13 @@ function getCatFact(): string {
return FACTS[Math.floor(Math.random() * FACTS.length)];
}

export default createPlugin({
type CatFactsDeps = {
logger: typeof LoggerToken,
};

type CatFactsService = void;

export default createPlugin<CatFactsDeps, CatFactsService>({
deps: {logger: LoggerToken},
middleware: deps => (ctx: Context, next: () => Promise<*>) => {
if(ctx.url === '/log-cat-fact') {
Expand All @@ -100,7 +106,11 @@ export default createPlugin({
});
```

Let's break this down. We import a `type {Context}` from `fusion-core` which is used in our middleware signature. This let's us ensure safe accessibility of the `ctx` object. We are also using the dependency injected logger which is registered with `LoggerToken`. If we look at the type of `logger`, we would expect to see:
Let's break this down.

The `createPlugin` function takes two generics `CatFactsDeps` and `CatFactsService`. The type `CatFactsDeps` specifies what should be the type of the `deps` property. The type `CatFactsService` specifies what should be the type of the service returned by `provides`. Since this example only sets up a middleware and doesn't provide a service API, we simply alias `CatFactsService` with `void`.

We import a `type {Context}` from `fusion-core` which is used in our middleware signature. This let's us ensure safe accessibility of the `ctx` object. We are also using the dependency injected logger which is registered with `LoggerToken`. If we look at the type of `logger`, we would expect to see:

```js
// defined in https://github.com/fusionjs/fusion-tokens/blob/master/src/index.js#L24
Expand All @@ -116,3 +126,77 @@ export type Logger = {
```

If we tried to do `deps.logger.notALogMethod(...)` we would expect a type error. Fusion.js will automatically hook up these injected types when they are available, ensuring more seamless type safety across the Fusion.js ecosystem.

#### Universal plugin types

It's possible for plugins to be non-trivial enough that they have similar but not identical type signatures on server and browser.

The recommended way to type those types of plugins is to create separate plugins and tokens for each environment:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, we don't follow this advice internally. There are a few places where we use the same types across environments when the actual types differ.

This is (1) partially due to retrofitting types after the fact, and (2) to avoid introducing new environment-specific tokens for the same plugins.

Copy link
Contributor Author

@lhorie lhorie Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm aware that we basically lie for our own plugins, but I think for new development, we should discourage people from following that practice. Thoughts?

Copy link
Member

@alxmyth alxmyth Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this introduce a new implicit dependency between the browser and node plugins? What happens if a user registers one of these tokens but not the other? With a single environment-agnostic token, we don't run into this problem today.

I wonder if this is maybe a good case for why we might want to revisit more helper utilities for creating and exporting plugins.

I could see something like export default exportPlugin(browserPlugin, nodePlugin) or createBrowserPlugin and createNodePlugin to obfuscate any typing magic we do away from the user.

Copy link
Contributor Author

@lhorie lhorie Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they'd be different problems, but I'm more on board w/ the issues of having two separate tokens for server/browser registrations. In that case, at best, you get an error saying "Such and such token was not registered" and at worst, the runtime simply does nothing because you forgot to add the code to do it.

With a single token overloading both server and client type, there's no way to correctly type either server-side or browser-side usage. At best you need to add type checks everywhere to disambiguate a A | B type, and at worst you get uncaught runtime type errors because you used a part of the server type signature in the browser or vice-versa.

Copy link
Member

@alxmyth alxmyth Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that from a static typing perspective, different tokens makes our lives easier as it avoids the need for any hacks or type refinements.

I don't think we have (or feasibly can) address the issue of the client and server implementations having an implicit dependency upon each other that isn't codified anywhere. We do not have any error handling around this if done today to say "you are missing the server implementation for this plugin" for example. It would be the worst case from above - runtime does not work as expected, since only half of the plugin is working. A cursory glance may confuse a user as it looks like the code is there.

That being said, this trade-off is possibly sufficient if we are looking to avoid any compiler hacks (e.g. utility functions to hide this) and want a cleaner static typing experience. I'd be interested to see what other folks' opinions are here, and whether we should adopt this approach for our plugins as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just a little concerned that we would give this advice if we don't follow it ourselves. If we are going to document this approach, we should probably update our existing plugins to as well. It might be worth making a list of migrations we would have to do to follow this?


```js
// src/plugins/foo/server.js
import type {FusionPlugin} from 'fusion-core';

export type ServerFooDeps = {
bar: typeof BarToken,
}

export class ServerFoo {
greet() {
console.log('hello');
}
}

export const ServerFooToken = createToken<ServerFoo>('Foo');
export const ServerFooPlugin = ((__NODE__ && createPlugin<ServerFooDeps, ServerFoo>({
deps: {bar: BarToken},
provides({bar}) {
return new ServerFoo();
}
}): any): FusionPlugin<ServerFooDeps, ServerFoo>);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we ok with this as a recommendation?


// src/plugins/foo/browser.js
import type {FusionPlugin} from 'fusion-core';

export type BrowserFooDeps = {
baz: typeof BazToken,
}

export class BrowserFoo {
dontGreet() {
console.log('no hi for you');
}
}

export const BrowserFooToken = createToken<BrowserFoo>('Foo');
export const BrowserFooPlugin = ((__BROWSER__ && createPlugin<BrowserFooDeps, BrowserFoo>({
deps: {baz: BazToken},
provides({baz}) {
return new BrowserFoo();
}
}): any): FusionPlugin<BrowserFooDeps, BrowserFoo>);

// src/plugins/foo/index.js
export * from './plugins/foo/server.js';
export * from './plugins/foo/browser.js';

// src/main.js
import App from 'fusion-react';
import {ServerFooToken, ServerFooPlugin, BrowserFooToken, BrowserFooPlugin} from './plugins/foo/index.js';

export default () => {
const app = new App();

if (__NODE__) {
app.register(ServerFooToken, ServerFooPlugin);
else {
app.register(BrowserFooToken, BrowserFooPlugin);
}

return app;
}
```

Note that we use `__NODE__ && ...` and `__BROWSER__ && ...` code fences to treeshake the plugins (and their dependencies) out of the appropriate bundles. The cast to `any` and back to `FusionPlugin` is required to prevent Flow from nagging about the environment selection boolean.

Warning: You should not move the `createPlugin` call outside of the `__NODE__ && ...` fence, as doing so can prevent treeshaking.