Skip to content

Conversation

@cehan-Chloe
Copy link

Divided into two commits

  1. add ts clientlib for swapi
  2. as ts language option

@cehan-Chloe cehan-Chloe requested a review from magicmark January 18, 2024 17:26
import _ from 'lodash';
import invariant from 'assert';
import DataLoader from 'dataloader';
import util from "util";
Copy link
Collaborator

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 ' -> "?)

Copy link
Author

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

*/

import fetch from 'node-fetch';
import * as url from 'url';
Copy link
Collaborator

@magicmark magicmark Feb 15, 2024

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
Comment on lines 6 to 9
export enum LanguageOptions {
FLOW = 'flow',
TYPESCRIPT = 'typescript',
}
Copy link
Collaborator

@magicmark magicmark Feb 15, 2024

Choose a reason for hiding this comment

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

nit: enum is fine i guess, but I think a union would be more idiomatic

Screenshot 2024-02-15 at 8 57 36 AM
Suggested change
export enum LanguageOptions {
FLOW = 'flow',
TYPESCRIPT = 'typescript',
}
export type LanguageOption = 'flow' | 'typescript';

Copy link
Collaborator

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)

}

export function getLoadersTypeMap(
language: LanguageOptions = LanguageOptions.FLOW,
Copy link
Collaborator

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

Copy link
Author

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()
}
}

Copy link
Collaborator

@magicmark magicmark left a 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants