-
Notifications
You must be signed in to change notification settings - Fork 12
Tsclientlib and language option #349
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.
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
base: master
Are you sure you want to change the base?
Conversation
import _ from 'lodash'; | ||
import invariant from 'assert'; | ||
import DataLoader from 'dataloader'; | ||
import util from "util"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know why the file formatting changed on this? probably fine but just curious if there's a new version of prettier or eslint or something causing all this (e.g. what's causing '
-> "
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite sure about this... prettier versions are not updated. how could we test if this will be an issue
examples/swapi/swapi.ts
Outdated
*/ | ||
|
||
import fetch from 'node-fetch'; | ||
import * as url from 'url'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the status quo uses the url
import but tbh this is probably a mistake, this is easy using stdlib (available as of node v10)
try the following in a node repl:
new URL('/people/123', 'https://swapi.dev/api/');
or if you don't want to rely on a magic .toString()
(new URL('/people/123', 'https://swapi.dev/api/')).href
src/config.ts
Outdated
export enum LanguageOptions { | ||
FLOW = 'flow', | ||
TYPESCRIPT = 'typescript', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also note the LanguageOptions
-> LanguageOption
(singular), since there's only one one option being decided here (the target language)
src/genTypeFlow.ts
Outdated
@@ -136,6 +136,7 @@ export function getLoaderType(resourceConfig: ResourceConfig, resourcePath: Read | |||
} | |||
|
|||
export function getLoadersTypeMap( | |||
language: LanguageOptions = LanguageOptions.FLOW, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, this file is called genTypeFlow.ts
implying this is specific for generating types for flow. in my mind, for doing typescript type generation, we'd just copy and paste this whole file and start again making it only typescript focused (rather than having each individual function do a little if statement to check are we generating for flow or typescript)
with that in mind, the choice of 'flow' here would be implicit and not needed as an argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we use separate genTypeFlow and genTypeTypesciprt, then we could move to codegen function check if the language option is flow or typescript. but in implementations, we'll probably still can't avoid these little if statements.. any suggestions?
export default function codegen(
/**
* The user specified config object, defining the shape and behaviour of
* the resources. May be arbitrarily nested, hence the 'any' type.
* (Read from dataloader-config.yaml)
*/
config: GlobalConfig,
...
) {
...
if config.language == typescript {
output = getTypescriptLoaders()
}
else {
output = getFlowLoaders()
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good! I think we can continue to add to this PR (or if you want to split things up, feel free to start a new branch or fork that we can merge to)
Divided into two commits