Skip to content

Conversation

jack-guy
Copy link
Contributor

@jack-guy jack-guy commented Jun 27, 2025

This PR drops Flow support in favor of TypeScript. Although dataloader-codegen is set up in a way to support multiple languages, given Flow's trajectory as a type system IMO the additional burden of maintaining two type systems simultaneously doesn't make sense.

Other changes of note:

  • Removed yarn.lock from this package - it makes it annoying to develop on internally (where we force network requests to hit our internal registry instead of npm's) and packages like ours don't generally need lockfiles
  • Bumps node support to latest LTS
  • I hadn't seen strip out swapi + namepsace to flow #354 but looks like someone is maintaining swapi.dev at swapi.info now and it works the same as before
  • Bumped test coverage to 100% and filled in the gaps
  • Bumped various dependencies to align with our internal registry versions

Verification

Running the swapi example works as expected. This also appears to be working as expected when linked into our internal services.

@jack-guy jack-guy requested a review from magicmark June 27, 2025 18:27
@jack-guy jack-guy force-pushed the migrate-to-typescript branch from 0fce080 to 524daec Compare June 27, 2025 18:28
`;
})()}

let response = await ${callResource(resourceConfig, resourcePath)}(resourceArgs);
// Any-type so this is re-assignable to the 'nestedPath' without TS complaining
Copy link
Collaborator

Choose a reason for hiding this comment

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

in theory this could use a supression instead

but then we don't necessarily know what linter or whatever we'll use in the future that might complain, which might have a different suppression syntax (e.g. maybe one day we switch to biome)

so i guess this is still good

@jack-guy jack-guy force-pushed the migrate-to-typescript branch from 2e8bc64 to 0c1d744 Compare June 27, 2025 23:22
Test using node 22
@jack-guy
Copy link
Contributor Author

@magicmark added compile-time tests for expect-type. I wasn't entirely sure how to structure all of this (ended up being convenient using the swapi example to do the type tests) so open to any feedback how to reorganize those tests.

In the process I think I uncovered a bug - we aren't supplying batch keys as a Set to resources when isBatchKeyASet: true is specified. This should only impact one of our internal dataloaders, and it looks it should be a no-op because our clientlib is calling Array.from() on the argument before making the API call. Let me know if I've misunderstood the intended behavior for this option though.

@jack-guy jack-guy requested a review from magicmark June 30, 2025 18:58
Comment on lines +26 to +27
// Check that the param type of isBatchKeyASet: true matches
expectTypeOf<Parameters<LoadersType['getFilms']['load']>>().branded.toEqualTypeOf<[args: { film_id: number }]>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

++++ nice

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.

Awesome!

This also appears to be working as expected when linked into our internal services.

++

Feel free to merge and follow https://github.com/Yelp/dataloader-codegen/blob/master/PUBLISH.md for a major change (let's go to 1.0.0, I think it's time :P)

@jack-guy jack-guy merged commit 392640d into master Jul 1, 2025
1 check passed
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.

2 participants