-
Notifications
You must be signed in to change notification settings - Fork 3
SWAPI, Paging, DataLoader, Request ID #242
base: master
Are you sure you want to change the base?
Conversation
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.
Please add an integration test. See joke test for reference (not e2e one)
Added integration test. |
src/connectors/swapi.ts
Outdated
}; | ||
|
||
/** | ||
* Given a type, get the object with the ID. |
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.
the function purpose can be easily derived from the name. does it make sense to rephrase the same in the comment?
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.
No, not at all. Comments left during code port. Removed.
src/connectors/swapi.ts
Outdated
}; | ||
|
||
/** | ||
* Given an objects URLs, fetch it, append the ID to it, sort it, and return it. |
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.
sort it
where?
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.
Removed useless and incorrect comments (left from another project source code),
src/graphql/models/swapi.ts
Outdated
/** | ||
* Given a type and ID, get the object with the ID. | ||
*/ | ||
const byTypeAndId = async (type: string, id: string): Promise<Object> => { |
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.
await
is kind of useless here, why not just
const byTypeAndId = async (type: string, id: string): Promise<Object> => getObjectFromTypeAndId(type, id)
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.
Refactored.
src/graphql/models/swapi.ts
Outdated
getObjectFromTypeAndId, | ||
getObjectsFromType, | ||
objectWithId, | ||
} from '../../connectors/swapi'; |
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.
To improve readability, I would suggest to import everything from the swapi connector and then just reference individual fns as such:
import * as swapi from '../../connectors/swapi';
....
const byTypeAndId = async (type: string, id: string): Promise<Object> =>
// immediately clear that this model fn simply delegates to the connector module
swapi.getObjectFromTypeAndId(type, id)
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.
Refactored.
src/graphql/models/swapi.ts
Outdated
|
||
objects = objects.concat(typeData.results.map(objectWithId)); | ||
while (nextUrl) { | ||
// eslint-disable-next-line no-await-in-loop |
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.
eslint
?
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.
Removed (left from another project source code).
src/graphql/resolvers/index.ts
Outdated
@@ -4,11 +4,15 @@ import { Resolvers } from '../_generated/types'; | |||
import { query as Query } from './Query'; | |||
import { jokes as Jokes } from './Jokes'; | |||
import { joke as Joke } from './Joke'; | |||
import { node as Node } from './Node'; | |||
|
|||
/** SWAPI resolvers */ |
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.
redundant comment
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.
Removed.
|
||
const resolvers: Resolvers = { | ||
Query, | ||
Jokes, | ||
Joke, | ||
}; | ||
|
||
export default merge(resolvers); | ||
export default merge(resolvers, swapiResolvers, { Node }); |
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.
why not spread over resolvers
?
const resolvers: Resolvers = {
Query,
Jokes,
Joke,
...swapiResolvers,
{ Node }
};
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.
Generated TypeScript interface Resolvers
does not contain SWAPI resolvers types.
import { importSchema } from 'graphql-import'; | ||
|
||
import resolvers from './resolvers'; | ||
|
||
const typeDefs: string = importSchema('src/graphql/schema/schema.graphql'); | ||
/** SWAPI schema */ | ||
import { swapiDef } from './schema/swapi'; |
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.
please covert all ts
schema files to graphql
SDL (sorry)
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.
It's not possible due to Relay connections. Please see ts schema files for details.
Added request id to logs. |
No description provided.