uber-web / thrift2flow Goto Github PK

Problem:
For example, if a file foo.thrift imports the file any.thrift and tries to references one of any.thrifts exports, the generated flow code will ultimately look like:

foo.js:

import any from './any';

type bar = {
  a: any.baz,
}

at which point flow will report an error because it doesn't expect a . after the keyword any. I imagine this might happen with other file names that are flow-protected keywords.

Thrift definition:

1: optional i64 (js.type = "Date") timestampMs;

Generated type:

timestampMs?: ?string,

while it should be:

timestampMs?: ?string | ?number,

According to https://github.com/thriftrw/thriftrw/blob/master/annotations.md#timestamps

will accept an ISO-8601 timestamp or milliseconds since the epoch (as returned by Date.now())

What is in thrift:

enum WorkflowStatus {
INACTIVE,
ACTIVE,
TEST,
SIMULATIONSCHEDULED = 10,
SIMULATIONACTIVE,
SIMULATIONINACTIVE
}

Output of thrift2flow:
export type WorkflowStatusType =
| "ACTIVE"
| "INACTIVE"
| "SIMULATIONACTIVE"
| "SIMULATIONINACTIVE"
| "SIMULATIONSCHEDULED"
| "TEST";
export const WorkflowStatusValueMap = {
ACTIVE: 1,
INACTIVE: 0,
SIMULATIONACTIVE: 4, <- expect this to be 11
SIMULATIONINACTIVE: 5, <- expect this to be 12
SIMULATIONSCHEDULED: 10,
TEST: 2
};

#16 and #17 added values to an enum map. This was useful if the code was generated by golang value enums, where array positions represent numbers in the code. #90 adds the function back.

from here: https://github.com/uber-node/thrift2flow/blob/master/src/main/types.js#L30-L40

Javascript doesn't have a 64 bit number representation. It is normally gets passed as a hex-based buffer: https://github.com/thriftrw/thriftrw-node#i64

Adding static typing for this product would hopefully decrease runtime breaks

thrift:

enum TicketType {
    A = 1
    B = 2
    C = 3
    D = 4
    E = 5
    F = 0
}

const set<i32> SOME_TICKET_TYPES = [
    TicketType.A,
    TicketType.B,
    TicketType.C,
    TicketType.F,
];

const set<i32> OTHER_TICKET_TYPES = [
    TicketType.A,
    TicketType.B,
    TicketType.C,
    TicketType.D,
    TicketType.E,
    TicketType.F,
];

js:

export type TicketTypeType =
  | 'A'
  | 'B'
  | 'C'
  | 'D'
  | 'E'
  | 'F';
export const TicketTypeValueMap = {
  A: 1,
  B: 2,
  C: 3,
  D: 4,
  E: 5,
  F: 0,
};

export const SOME_TICKET_TYPES: number[] = [
  TicketType.A,
  TicketType.B,
  TicketType.C,
  TicketType.F,
];

export const OTHER_TICKET_TYPES: number[] = [
  TicketType.A,
  TicketType.B,
  TicketType.C,
  TicketType.D,
  TicketType.E,
  TicketType.F,
];

Note, TicketType.A, TicketType.B should be TicketTypeValueMap.A, TicketTypeValueMap.B.

Adding continuous integration will help us know if this product breaks or not

It seems as though there's a breaking change in thrift2flow 0.11.0 where maps become exact width maps in the js files:

3: optional map<string,string> myStringMap

Used to be this in 0.9.0

myStringMap?: ?{ [string]: string }

Is now this in 0.11.0

myStringMap?: ?{| [string]: string |}

This is a breaking change if flowtype was checking the types of assigned objects before. Afterward it produces this error:

Cannot assign Object.freeze(...) to pizza because property minSpend is missing in object type [1] but exists in object
literal [2] in property someVariable.myStringMap.

Currently, when generating the type file a source path is prepended before the types. This is not useful when keeping the types in a remote repo as multiple users can change this from multiple relative paths.

There is currently no way to configure the outputDir from the cli.

I noticed that the thriftrw appears to parse the date into a string of keep it as a native javascript date.

My thrift defintions are as follows

typedef i64 (js.type = "Date") Timestamp

struct Something {
  1: required i32 id
  2: required types.Timestamp createdAt
}

Expected Output

  export type TimestampType = string;

export type SomethingType = {|
  id: number,
  createdAt: types.TimestampType,
|};

Actual Output

  export type TimestampType = Date;

export type SomethingType = {|
  id: number,
  createdAt: types.TimestampType,
|};

Should we add special types for thriftrw so a set of flow types is created to match thrift spec and a set of flow types is generated to create types returned by thriftrw.

  export type TimestampType = Date;
  export type TimestampTypeRW = string;

export type SomethingType = {|
  id: number,
  createdAt: types.TimestampType,
|};
export type SomethingTypeRW = {|
  id: number,
  createdAt: types.TimestampTypeRW,
|};

The end result is that my generators for test data is passing flow validation because it is creating a string instead of a date.

When translating thrift enums like so:

enum EntityErrorType {
    UNKNOWN = 0,
    NOT_EXIST,
    EXISTED,
}

Thrift2Flow generates two objects

export type EntityErrorTypeTypeValues = 0 | 1 | 2;
export type EntityErrorTypeType = 'UNKNOWN' | 'NOT_EXIST' | 'EXISTED';

Is there a way in flow to map these back to one another through flow or through this tool? Would it make more sense for this tool to generate out a single object like so?

export type EntityErrorTypeType = {
   'UNKNOWN': 0,
   'NOT_EXIST': 1,
   'EXISTED': 2
};

thrift:

const map<string, Category> UUID_TO_CATEGORY = {
    "31bf328d-8456-8a8e-3b31-64f3dc79c090": Category.ACCOUNTING,
    "591d1392-09ea-a730-7bb8-89b622467cb0": Category.ADULT_ENTERTAINMENT,
    "b03b5536-61f8-1144-4c0d-1dac1a79534f": Category.AFGHAN
}

js:

export const UUID_TO_CATEGORY: {
  [string]: CategoryType,
} = undefined;

Since this product doesn't get touched a whole ton, it would be good to run renovate on it. The only sticking point here is that we may need to move the flow-bin dependency out to a peer dependency or lock it to something lower.

Flow doesn't have statistics on what versions are currently in use so it's hard to know what the lower bound of support is.

Flow also isn't at a major build so we can be broken at any time.

The other choice is to always depend on the latest flow-bin and if someone is using an older flow-bin and we output something that isn't compatible, it's up to them to resolve it. This is probably the easiest from a maintenance standpoint until we have major builds that are backwards compatible.

Also what about js.type annotation. Is it supported?

typedef i64 (js.type = 'Date') Timestamp
typedef i64 (js.type = 'Long') TimeDelta
typedef i64 (js.type = 'Long') Long
typedef string Decimal

For example, if I have:
optional string foo
in Thrift, this should map to:
foo: ?string
Or maybe
foo?: ?string
Instead of:
foo?: string

Currently, this prevents null from being a valid value for the field (in JS), whereas in Thrift null IS a valid value.

Thrift enum:

enum SomeEnum {
    A = 1,
    B = 2,
    C = 3,
}

I need (as in response from Java code):

const SomeEnum = {
  A: 1,
  B: 2,
  C: 3,
}

but getting:

const SomeEnum = {
  A: 'A',
  B: 'B',
  C: 'C',
}

I think thriftrw uses strings as values (https://github.com/thriftrw/thriftrw-node#enums), but whatever is used on the Java server returns numbers, so I guess this should generate both variants.

Right now there seems to be a bit of a discrepancy of data being generated due to the source of data and what this tool actually targets. For example, the data contracts from the server who owns the thrift contract and the browser itself looks something like this thrift -> tchannel -> thriftrw -> browser where at any arrow the data contract can (and does) change.

Right now thrift2flow really focuses on that last arrow thriftrw -> browser. But it bases it's information from the thrift file directly which it gets from the server. In reality, it should base it's information off the tchannel translation of this thrift file definition.

Based on this there needs to be a few tools written

1. Flow generator based on the thrift file itself (possible integration with the thrift product similar to how they did typescript)
2. thriftflow2tchannel converter
3. tchannel2thriftrw converter

This would then allow for the most accurate portral of the conversions going on along with if a layer was removed in the middle somewhere, most accurate maintenance of final generation.

Currently, the enums are converted to regular JavaScript objects. If they were wrapped with Object.freeze then Flow could easily infer object shape at any moment. Bonus points for auto-creating a $Keys and $Values type from all enums.

Some thrift services use Consts in place of enums.

It would be great if we could add a case to handle that.

example:

input

const string STATUS_ELIGIBLE       = "eligible"

output

export const STATUS_ELIGIBLE: string = 'eligible';

Ran npm install -g thrift2flow
Running thrift2flow throws this error:

Error: Cannot find module 'mkdirp'
    at Function.Module._resolveFilename (module.js:548:15)
    at Function.Module._load (module.js:475:25)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/jonsadka/.nvm/versions/node/v8.12.0/lib/node_modules/thrift2flow/lib/index.js:24:15)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)

Running npm list -g --depth=0 shows that [email protected] is installed

NOTE: Tried on node 8.12.0 and node 6.10.3

The community standards don't seem to align with Uber's community standards

• Need to lock down master so people can't push directly to it
• Need to add issue templates
• Need to add pull request templates
• Need to add Code of conduct
• Need to add Contributing guidelines

I'd like to provide a --prefix , such that a Template structure in thrift could be named CG_Alpha_Template. In that example, --prefix "CG_Alpha_" helps label it as "CG" as code-gen and "Alpha" as the service name.

Historically, all fields in our back-end service thrift files are optional and the front-end defines a new flow type for just the necessary fields. On top of that, two thrift files may have identical type names. Therefore, Template type can actually be the name of four unique types (4) in a particular project.

Instead, it would be nice if the four unique types had four different names so they are easily searchable by "_Template" or "Alpha_Template", but actually named:

• CG_Alpha_Template vs CG_Zeta_Template in flow type code-gen
• Alpha_Template vs Zeta_Template in our own types that are a subset of code-gen

The renaming could also be done as part of post-processing in the code-gen CLI that uses this thrift2flow library. The code-gen CLI would need some changes whether it passes a --prefix or does post-processing, so I'm also opening an issue there.

Please let me know what you think! Thank you.

Running thrift2flow ./idl/code.uber.internal/**/*.thrift works, but if I add it to package.json's scripts:

"idl-flow": "thrift2flow ./idl/code.uber.internal/**/*.thrift"

it stops working. I believe it's because my shell expands the glob expression while the scripts in package.json do not (see https://medium.com/@jakubsynowiec/you-should-always-quote-your-globs-in-npm-scripts-621887a2a784). It would be good to have an ability to pass either a folder or a glob to the CLI.

If you have a file goautobots.thrift of which has no dependencies thrift2flow will output flow-output/goautobots.js. If goautobots.thrift then has a dependency include "../floweco/floweco.thrift" then the output file is flow-output/goautobots/goautobots.js. This shift makes adding tooling ontop of thrift2flow (e.g. moving the files) dependent on what the service does and can break scripts easily.

Possible alternative is to output flow-output/goautobots.js and flow-output/goautobots/* where goautobot's dependencies are in the subfolder.

Example thrift definition

struct UserActivitiesRequest {
    10: required string userUUID
    20: optional list<string> workflowUUIDs
    30: optional i64(js.type = "Long") fromTimestampNano
    40: optional i64(js.type = "Long") toTimestampNano
}

Current output

export type UserActivitiesRequestType = {|
  fromTimestampNano?: Long,
  toTimestampNano?: Long,
  userUUID: string,
  workflowUUIDs?: string[],
|};

Expected output

import {default as Long} from 'long';
export type UserActivitiesRequestType = {|
  fromTimestampNano?: Long,
  toTimestampNano?: Long,
  userUUID: string,
  workflowUUIDs?: string[],
|};

When converting a Thrift file that includes Symbols, the property name is eventually replaced with the word "symbol" and it breaks when Flow tries to type check the file.

For instance, this thrift file includes a Var type with a name property that is a Symbol:

other_file.thrift

struct Var {
  1: required Symbol name
  2: required Expression expression
}

main_file.thrift

struct Variable {
    1: optional other_file.Symbol symbol
    2: optional other_file.Expression expression
}

This gets converted to the following JS file:

other_file.js

export type _Symbol = string;
export type Var = {| name: _Symbol, expression: Expression |};

Then this gets imported and used, but in a broken way:

main_file.js

import * as other_file from "./other_file";

export type Variable = {|
  symbol?: ?other_file.Symbol,
  expression?: ?other_file.Expression
|};

Notice that the Variable type doesn’t include “name” as a property, instead it uses “symbol” and the type is other_file.Symbol.

Flow then fails with the following:

Cannot get other_file.Symbol because property Symbol is missing in module
./other_file [1].

 [1]  3│ import * as other_flow_file from "./other_file";
       :
     25│ |};
     26│
     27│ export type Variable = {|
     28│   symbol?: ?other_file.Symbol,
     29│   expression?: ?other_file.Expression
     30│ |};
     31│

Thank you for this library!

I notice the generated output changed from nameType => name, which means we needed to update usages. Is this a bug or intentional? Without 'Type', types look like regular variable names in end-user code and inconsistent with custom type names following the nameType convention.

Also it looks like other projects like base Web follow a third convention, nameT, and I was wondering if there could be a consensus.

Right now thrift2flow creates a directory and outputs the file there. This makes scripting a bit harder since you can't take the contents of the output and perform operations on it. Some examples:

1. What if I want to output it to a different location? Right now I have to run thrift2flow, copy the file and then delete the folder created by the tool.
2. Testing currently depends on the file system since it's actually outputting files as the tests run.

Instead I suggest when you run

thrift2flow something.thrift

That the contents of the generated file gets outputted to the console and when you run

thrift2flow something.thrift --o[[ut]put] ../something.types.js

The output will be written to the file path provided

An extension of this would be the ability to pass globby style strings and output globby style outputs. For example

// Filesystem
./thrifts/group1/subgroup1/product1.thrift
./thrifts/group1/subgroup2/product1.thrift
./thrifts/group1/subgroup2/product2.thrift

// CLI
thrift2flow ./thrifts/**/*.thrift --o[[ut]put] ./types/**/*.types.js

But this could be handled as a separate issue

babel-polyfill is imported in the index.js: https://github.com/uber-node/thrift2flow/blob/master/src/main/index.js#L31

I'm getting:

➜  fievel git:(T1215207-suggest-edit-endpoint) thrift2flow ./<my-file>
Error: Cannot find module 'babel-polyfill'
    at Function.Module._resolveFilename (module.js:536:15)
    at Function.Module._load (module.js:466:25)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/fspiridonov/.nvm/versions/node/v8.9.0/lib/node_modules/thrift2flow/lib/index.js:6:1)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)

It should be moved to the dependencies