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.

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

Static Typing with Flow #8

Closed
wants to merge 7 commits into from
Closed

Static Typing with Flow #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b4130ea
inital infra for flow typing
Mar 20, 2016
10afb89
correct case for string definitions and added type checking to cacheU…
Mar 20, 2016
636682d
added flow to all current src files
Mar 20, 2016
b2d6c3e
switch to using operationDefinition instead of definition
Mar 20, 2016
50f5c20
minor code style fix
Mar 20, 2016
80661f4
added in more finely grained typeing for destructured arguments
Mar 20, 2016
a5ca9cb
only allow strings as id per the relay server spec
Mar 20, 2016
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
3 changes: 2 additions & 1 deletion .babelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
"presets": [
"stage-0",
"es2015"
]
],
"plugins": ["transform-flow-strip-types"]
}
2 changes: 2 additions & 0 deletions .flowconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[libs]
interfaces/
8 changes: 6 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
"jsnext:main": "./src/index.js",
"scripts": {
"test": "mocha --reporter spec --full-trace test/index.js",
"posttest": "npm run lint",
"posttest": "npm run lint && npm run flow",
"compile": "babel --presets es2015,stage-0 -d lib/ src/",
"prepublish": "npm run compile",
"lint": "eslint ."
"lint": "eslint .",
"flow": "flow; test $? -eq 0 -o $? -eq 2",
"watch": "babel --watch=./src --out-dir=./lib"
Copy link
Contributor

Choose a reason for hiding this comment

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

What do we need watch for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stubailo I added it as a helper so you can live typecheck as you work. With flow being in the babelrc, on compile it typechecks. I can remove it though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting! I don't have anything against it, just thought it wouldn't do anything since I thought you needed to explicitly npm run flow.

I assume there are also IDE/editor integrations that will run the typechecking for you in the editor, like you get with linting? That would be what I really want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah both atom and VSCode have typechecking in the editor for flow

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve just tried Flow integration in VSCode but I wasn’t able to make it work. The extension is developed by the Flow core team at Facebook, which is a good sign, but untouched for the past 4 months which is a bit sad.

https://github.com/flowtype/flow-for-vscode

},
"repository": {
"type": "git",
Expand All @@ -36,13 +38,15 @@
"babel-core": "6.3.21",
"babel-eslint": "^6.0.0-beta.6",
"babel-loader": "6.2.0",
"babel-plugin-transform-flow-strip-types": "^6.7.0",
"babel-preset-es2015": "^6.5.0",
"babel-preset-stage-0": "^6.5.0",
"chai": "^3.5.0",
"dataloader": "^1.1.0",
"eslint": "^2.4.0",
"eslint-config-airbnb": "^6.1.0",
"eslint-plugin-import": "^1.1.0",
"flow-bin": "^0.22.1",
"graphql": "^0.4.17",
"graphql-relay": "^0.3.6",
"lodash": "^4.5.1",
Expand Down
8 changes: 6 additions & 2 deletions src/cacheUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
export function cacheFieldNameFromSelection(selection) {
// == `cacheUtils.js` == //
// @flow
import type { Selection } from 'graphql/language/ast';

export function cacheFieldNameFromSelection(selection: Selection): string {
if (selection.arguments.length) {
const argObj = {};
selection.arguments.forEach((argument) => {
Expand All @@ -11,7 +15,7 @@ export function cacheFieldNameFromSelection(selection) {
return selection.name.value;
}

export function resultFieldNameFromSelection(selection) {
export function resultFieldNameFromSelection(selection: Selection): string {
return selection.alias ?
selection.alias.value :
selection.name.value;
Expand Down
6 changes: 4 additions & 2 deletions src/debug.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
function stripLoc(obj) {
// == `debug.js` == //
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot for the life of me figure out why these comments are here (in general, I'm tearing my hair out trying to get Flow and Nuclide to work together). What are they for? They seem to actually break Flow because it seems to expect the @flow comment to be at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stubailo I can't remember now either. I think maybe the linter in atom added them? I had type errors with flow for normal types and I'm pretty sure I had them working with GraphQL types. I haven't tried nuclide yet though.

I'll get it back running locally and debug! Sorry about that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stubailo I was pulling from the graphql source to get the type definitions since they aren't included in the distro. The easy way around this until they include it is to do a flow type def just like we have for typescript. Shouldn't take me long to implement 👍 so we can fully evaluate them both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh, that would explain why there were no errors.

// @flow
function stripLoc(obj: Object) {
// For development only!
const _ = require('lodash');
if (! _.isObject(obj)) {
Expand All @@ -12,6 +14,6 @@ function stripLoc(obj) {
});
}

export function printAST(fragAst) { // eslint-disable-line no-unused-vars
export function printAST(fragAst: Object) { // eslint-disable-line no-unused-vars
console.log(JSON.stringify(stripLoc(fragAst), null, 2)); // eslint-disable-line no-console
}
22 changes: 15 additions & 7 deletions src/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
// == `parser.js` == //
// @flow
import { parse } from 'graphql/language';

import type {
Document,
OperationDefinition,
} from 'graphql/language/ast';

import { isString } from 'lodash';

export function parseIfString(doc) {
let parsed = doc;
export function parseIfString(doc: Document | string): Document {
let parsed: Document | string = doc;

if (isString(doc)) {
parsed = parse(doc);
Expand All @@ -15,8 +23,8 @@ export function parseIfString(doc) {
return parsed;
}

export function parseFragmentIfString(fragment) {
const parsedFragment = parseIfString(fragment);
export function parseFragmentIfString(fragment: Document | string): Document {
const parsedFragment: Document = parseIfString(fragment);

if (parsedFragment.definitions.length !== 1) {
throw new Error('Must have exactly one definition in document.');
Expand All @@ -31,14 +39,14 @@ export function parseFragmentIfString(fragment) {
return fragmentDef;
}

export function parseQueryIfString(query) {
const parsedQuery = parseIfString(query);
export function parseQueryIfString(query: Document | string): OperationDefinition {
const parsedQuery: Document = parseIfString(query);

if (parsedQuery.kind !== 'Document' && parsedQuery.definitions.length !== 1) {
throw new Error('Must have exactly one definition in document.');
}

const queryDefinition = parsedQuery.definitions[0];
const queryDefinition: OperationDefinition = parsedQuery.definitions[0];

if (queryDefinition.operation !== 'query') {
throw new Error('Definition must be a query.');
Expand Down
31 changes: 25 additions & 6 deletions src/readFromStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// == `readFromStore.js` == //
// @flow
import {
isArray,
has,
Expand All @@ -13,12 +15,21 @@ import {
resultFieldNameFromSelection,
} from './cacheUtils';

import type {
Document,
Definition,
SelectionSet,
} from 'graphql/language/ast';

// import {
// printAST,
// } from './debug';

export function readQueryFromStore({ store, query }) {
const queryDef = parseQueryIfString(query);
export function readQueryFromStore({
store,
query,
}: { store: Object, query: Document | string }): Object {
const queryDef: Definition = parseQueryIfString(query);

return readSelectionSetFromStore({
store,
Expand All @@ -27,7 +38,11 @@ export function readQueryFromStore({ store, query }) {
});
}

export function readFragmentFromStore({ store, fragment, rootId }) {
export function readFragmentFromStore({
store,
fragment,
rootId,
}: { store: Object, fragment: Document | string, rootId: string }): Object {
const fragmentDef = parseFragmentIfString(fragment);

return readSelectionSetFromStore({
Expand All @@ -37,13 +52,17 @@ export function readFragmentFromStore({ store, fragment, rootId }) {
});
}

function readSelectionSetFromStore({ store, rootId, selectionSet }) {
function readSelectionSetFromStore({
store,
rootId,
selectionSet,
}: { store: Object, rootId: string, selectionSet: SelectionSet }): Object {
if (selectionSet.kind !== 'SelectionSet') {
throw new Error('Must be a selection set.');
}

const result = {};
const cacheObj = store[rootId];
const result: Object = {};
const cacheObj: Object = store[rootId];

selectionSet.selections.forEach((selection) => {
const cacheFieldName = cacheFieldNameFromSelection(selection);
Expand Down
38 changes: 23 additions & 15 deletions src/writeToStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// == `writeToStore.js` == //
// @flow
import {
isString,
isNumber,
Expand All @@ -17,6 +19,12 @@ import {
resultFieldNameFromSelection,
} from './cacheUtils';

import type {
Document,
OperationDefinition,
SelectionSet,
} from 'graphql/language/ast';

// import {
// printAST,
// } from './debug';
Expand All @@ -35,14 +43,14 @@ export function writeFragmentToStore({
result,
fragment,
cache = {},
}) {
}: { result: Object, fragment: Document | string, cache: Object }): Object {
// Argument validation
if (!fragment) {
throw new Error('Must pass fragment.');
}

const parsedFragment = parseFragmentIfString(fragment);
const selectionSet = parsedFragment.selectionSet;
const parsedFragment: Document = parseFragmentIfString(fragment);
const selectionSet: SelectionSet = parsedFragment.selectionSet;

return writeSelectionSetToStore({
result,
Expand All @@ -55,10 +63,10 @@ export function writeQueryToStore({
result,
query,
cache = {},
}) {
const queryDefinition = parseQueryIfString(query);
}: { result: Object, query: Document | string, cache: Object }): Object {
const queryDefinition: OperationDefinition = parseQueryIfString(query);

const resultWithDataId = {
const resultWithDataId: Object = {
__data_id: 'ROOT_QUERY',
...result,
};
Expand All @@ -74,20 +82,20 @@ function writeSelectionSetToStore({
result,
selectionSet,
cache,
}) {
}: { result: Object, selectionSet: SelectionSet, cache: Object }): Object {
if (! isString(result.id) && ! isString(result.__data_id)) {
throw new Error('Result passed to writeSelectionSetToStore must have a string ID');
}

const resultDataId = result['__data_id'] || result.id;
const resultDataId: string = result['__data_id'] || result.id;

const normalizedRootObj = {};
const normalizedRootObj: Object = {};

selectionSet.selections.forEach((selection) => {
const cacheFieldName = cacheFieldNameFromSelection(selection);
const resultFieldName = resultFieldNameFromSelection(selection);
const cacheFieldName: string = cacheFieldNameFromSelection(selection);
const resultFieldName: string = resultFieldNameFromSelection(selection);

const value = result[resultFieldName];
const value: any = result[resultFieldName];

if (isUndefined(value)) {
throw new Error(`Can't find field ${resultFieldName} on result object ${resultDataId}.`);
Expand All @@ -101,10 +109,10 @@ function writeSelectionSetToStore({

// If it's an array
if (isArray(value)) {
const thisIdList = [];
const thisIdList: Array<string> = [];

value.forEach((item, index) => {
const clonedItem = { ...item };
const clonedItem: Object = { ...item };

if (! isString(item.id)) {
clonedItem['__data_id'] = `${resultDataId}.${cacheFieldName}.${index}`;
Expand All @@ -126,7 +134,7 @@ function writeSelectionSetToStore({
}

// It's an object
const clonedValue = { ...value };
const clonedValue: Object = { ...value };
if (! isString(clonedValue.id)) {
// Object doesn't have an ID, so store it with its field name and parent ID
clonedValue['__data_id'] = `${resultDataId}.${cacheFieldName}`;
Expand Down