flow-runtime icon indicating copy to clipboard operation
flow-runtime copied to clipboard

Proposal: use ref(() => Map) instead of ref('Map')

Open jedwards1211 opened this issue 8 years ago • 11 comments

This is a:

  • [ ] Bug Report
  • [ ] Feature Request
  • [x] Question
  • [ ] Other

Which concerns:

  • [x] flow-runtime
  • [x] babel-plugin-flow-runtime
  • [ ] flow-runtime-validators
  • [ ] flow-runtime-mobx
  • [ ] flow-config-parser
  • [ ] The documentation website

Usingref with the name of a type as a string is fundamentally problematic

Because the babel module transform mangles the names of imports. It has to because it can't know at compile time if the value of an export will change, because of circular imports, etc.

So any code that babel-plugin-flow-runtime converts to ref('Map') will break if Map comes from import Map from '...' because that import will get transpiled to

var _map = require('...')

var _map2 = _iteropRequireDefault(_map)

(see #177)

I think the best way to get around this would be to use a function instead of a string: ref(() => Map). That way it can still be evaluated at runtime, but babel's module transform will convert that to ref(() => _map2.default), so it won't break.

@phpnode thoughts?

jedwards1211 avatar Nov 29 '17 23:11 jedwards1211

We use a string ref (in this case) because at this stage we don't know whether Map is a concrete value or a type. I guess we could generate something like t.ref(() => typeof Map === 'undefined' ? 'Map' : Map) though

phpnode avatar Nov 30 '17 17:11 phpnode

Oh, I see. Well babel-plugin-flow-runtime is converting types to concrete values anyway; only types imported from external code compiled without babel-plugin-flow-runtime would not be a concrete value. So wouldn't t.ref(() => typeof Map === 'undefined' ? null : Map) work?'

And if it does work, really, we could just use () => Map and have flow-runtime catch the TypeError if Map is undefined.

jedwards1211 avatar Nov 30 '17 19:11 jedwards1211

babel-plugin-flow-runtime is converting types to concrete values anyway;

This is true most of the time, but not for global type definitions, for example if something is an Iterable<T> we don't import that directly and can't reference it by value, instead we use the type's name to look it up in our registry. The registry is built by running flow-runtime-cli. So we can't assume that every type will be a value we can directly reference, and if we can't reference the value we need a good error message telling the user which type is missing, so we do need the type name as a string.

phpnode avatar Dec 01 '17 10:12 phpnode

Ah, that makes sense. I see that registerBuiltins refers to Map directly (https://github.com/codemix/flow-runtime/blob/master/packages/flow-runtime/src/registerBuiltins.js#L67) so actually, if I processed flow-runtime with babel-plugin-transform-runtime in my code I bet it would work as-is.

If you don't mind my asking though, why not just import a pre-built runtime type directly instead of using t.ref('Iterable')? For instance:

Input

import {reify} from 'flow-runtime'
import type {Type} from 'flow-runtime'

const foo = (reify: Type<Iterable>)
const bar = (reify: Type<Map>)

Output

import { reify } from 'flow-runtime';
import { Type as _Type } from 'flow-runtime';
import { Iterable, Map as _Map } from 'flow-runtime'; // or maybe flow-runtime/builtins

import t from 'flow-runtime';
const Type = t.tdz(() => _Type);
const foo = Iterable;
const bar = _Map;

Even better, for Map, Set, and other concrete builtins, flow-runtime could actually accept the instance that's on the user's scope, which would solve the issue I was running into for sure:

const bar = _Map(Map);

jedwards1211 avatar Dec 01 '17 13:12 jedwards1211

This will work for known builtins but not anything that's declared in a file in the flow-typed directory, e.g. if you use express it's relatively common to use express$Request without directly importing it.

Regarding your second point - that should already be the case, for instance if you declare your own Map then flow-runtime will use it:

type Map = 123;

const map:Map = 123

compiles to:

import t from "flow-runtime";
const Map = t.type("Map", t.number(123));


const map = Map.assert(123);

This doesn't work in your scenario because babel-plugin-flow-runtime is running before babel-plugin-transform-runtime and so doesn't see the new reference to corejs.Map.

The only way to solve these issues nicely is to build webpack plugins / loaders which have access to the entire codebase, not just a single file at a time like babel does. I want to work on this but finding the time to do so is hard!

phpnode avatar Dec 01 '17 13:12 phpnode

So I webpack my client code, but I don't use it on my server-side code at all; I only babelify my server-side code. I'm sure many people do the same. So I don't think webpack plugins / loaders would really be a complete solution for all JS.

Does flow-runtime currently have a way of supporting express$Request? If so, how? Maybe it's common to use it but I think it's unwise and not worth supporting because it's essentially an undocumented type declaration polluting the global namespace that's subject to change without notice.

jedwards1211 avatar Dec 06 '17 06:12 jedwards1211

As far as I understood, flow-runtime can't currently import runtime types from any module that doesn't export concrete runtime types (which it has already converted with babel-plugin-flow-runtime)?

The thing I was talking about with const bar = _Map(Map); was not concretely declaring a Map type like type Map = 123 but passing in the Map that's been put on the global namespace by whatever polyfills are being used (if any), so that we can make sure flow-runtime is using the same class.

jedwards1211 avatar Dec 06 '17 06:12 jedwards1211

Here's an idea: how about running a server in the background that caches information about types in all the source files, and requesting info about other files in the babel plugin?

(I know you're busy and I'm not demanding that you to implement any of these things yourself; I'm just discussing the design because I might eventually try to implement a solution myself)

jedwards1211 avatar Dec 06 '17 06:12 jedwards1211

I quite like the idea but I also don't want to reinvent Flow, the best solution would be to make Flow's type information accessible at the AST level in babel, but that's quite a challenge

phpnode avatar Dec 06 '17 12:12 phpnode

Yeah I know, I just don't really see how else we would do that.

Here's another thought: we could have a preprocessing program go through all the type imports, and generate a file with all those types converted to validators if they aren't already.

The Babel plugin would then have only one place to look for importing these extra validators.

jedwards1211 avatar Dec 06 '17 17:12 jedwards1211

Here's another thought: we could have a preprocessing program go through all the type imports, and generate a file with all those types converted to validators if they aren't already.

That's what flow-runtime-cli does, so if you run flow-runtime generate ./src > ./src/typedefs.js it will crawl all your code and generate definitions in one file that you can later import. It is definitely not perfect though

phpnode avatar Dec 06 '17 21:12 phpnode