SWAPI, Paging, DataLoader, Request ID#242
Conversation
trioletas
left a comment
trioletas
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
await is kind of useless here, why not just
const byTypeAndId = async (type: string, id: string): Promise<Object> => getObjectFromTypeAndId(type, id)
src/graphql/models/swapi.ts
Outdated
| getObjectFromTypeAndId, | ||
| getObjectsFromType, | ||
| objectWithId, | ||
| } from '../../connectors/swapi'; |
There was a problem hiding this comment.
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)
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.
Removed (left from another project source code).
src/graphql/resolvers/index.ts
Outdated
| import { joke as Joke } from './Joke'; | ||
| import { node as Node } from './Node'; | ||
|
|
||
| /** SWAPI resolvers */ |
| }; | ||
|
|
||
| export default merge(resolvers); | ||
| export default merge(resolvers, swapiResolvers, { Node }); |
There was a problem hiding this comment.
why not spread over resolvers ?
const resolvers: Resolvers = {
Query,
Jokes,
Joke,
...swapiResolvers,
{ Node }
};There was a problem hiding this comment.
Generated TypeScript interface Resolvers does not contain SWAPI resolvers types.
|
|
||
| const typeDefs: string = importSchema('src/graphql/schema/schema.graphql'); | ||
| /** SWAPI schema */ | ||
| import { swapiDef } from './schema/swapi'; |
There was a problem hiding this comment.
please covert all ts schema files to graphql SDL (sorry)
There was a problem hiding this comment.
It's not possible due to Relay connections. Please see ts schema files for details.
alexbaluhin
changed the title
|
Added request id to logs. |
2 participants
No description provided.