Skip to content

Port DataLoader to TypeScript#385

Open
NShahri wants to merge 18 commits into
graphql:mainfrom
NShahri:typescript
Open

Port DataLoader to TypeScript#385
NShahri wants to merge 18 commits into
graphql:mainfrom
NShahri:typescript

Conversation

@NShahri

@NShahri NShahri commented Oct 30, 2025

Copy link
Copy Markdown

This PR migrates the entire codebase from JavaScript/Flow to TypeScript, modernizing the stack and improving type safety, tooling, and maintainability.

✅ Changes Included

  • Migrated all source and test files from .js to .ts
  • Removed Flow types and replaced them with TypeScript equivalents
  • Added tsconfig.json with strict compiler options
  • Switched build system from Babel to typescript and node type stripping
  • Support ESM
  • Refactored tests to TypeScript with proper type annotations
  • Updated package.json scripts and dependencies
  • Upgrade Eslint config
  • Removed obsolete files
  • switch to npm

📈 Benefits
This migration lays the foundation for better maintainability, improved tooling support, and enhanced type safety.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Oct 30, 2025

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

@NShahri

NShahri commented Oct 30, 2025

Copy link
Copy Markdown
Author

Hello, @leebyron @saihaj @graphql/dataloader-maintainers. This PR completes a full migration to TypeScript and includes several improvements to the build and test setup. I'd appreciate your review and any suggestions you might have. Thank you!

@jgcmarins jgcmarins left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Amazing

@ardatan ardatan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CI seems failing. Could you take a look?

@NShahri NShahri requested a review from ardatan November 3, 2025 05:27
@NShahri

NShahri commented Nov 3, 2025

Copy link
Copy Markdown
Author

Thanks @ardatan — I’ve fixed the CI issue. Could you please try re-running the workflow when you get a chance?

@ardatan

ardatan commented Nov 3, 2025

Copy link
Copy Markdown
Member

@NShahri Still failing :/

Comment thread jest.config.ts Outdated
Comment on lines +10 to +13
global._onUnhandledRejection = handler => {
_.on('unhandledRejection', handler);
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FAIL src/__tests__/unhandled.test.ts
  ● Unhandled rejections › Not catching a primed error is an unhandled rejection

    TypeError: global._onUnhandledRejection is not a function

I think the issue is that this needs to be done in a "jest environment" rather than in the config file; the jest workers don't run with this (unless you have jest -i maybe?) IIRC.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @benjie @ardatan

  • Unfortunately, it wasn’t possible to add unhandledRejection handler in Jest, even when using custom Jest environments. Because of this limitation, I decided to switch to Vitest.
  • Additionally, I realized mocking global dependencies like process or setTimeout was difficult since they were required at module import time. To make mocking easier, I refactored the code so that these dependencies are accessed only when the DataLoader is initialized.

Please check the commit and let me know your thoughts.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still failing unfortunately.

@NShahri NShahri Nov 5, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

  • Handling unhandledRejection in jest seems quite tricky— based on my research, it’s nearly impossible to do reliably. The only workaround I found is this approach, but it’s not ideal and feels a bit hacky.
  • On the other hand, vitest does support handling unhandledRejection, but it requires NodeJs 20+, which might limit compatibility.
  • Please correct me if I’m mistaken, but it looks like unhandled.test.ts is testing NodeJs behavior rather than application logic. I'm still unsure about the value of testing what happens when a rejected promise isn’t handled by user code. If that’s the case, perhaps this test could be removed or skipped?
  • Also worth noting: there’s already a test that checks whether the priming error returns a rejected promise, which might be sufficient.

Would it make sense to remove or skip the unhandled.test.ts? Or is there a specific scenario we’re trying to safeguard against that I might be missing?

@productdevbook

Copy link
Copy Markdown

tsup -> tsdown(https://tsdown.dev/) I definitely recommend using this.

@benjie

benjie commented Nov 8, 2025

Copy link
Copy Markdown
Member

Wouldn’t it be better to use Node’s type stripping for development, tsc for build, and leave the bundling up to the consumer?

@productdevbook

productdevbook commented Nov 8, 2025

Copy link
Copy Markdown

The reason for the transition. The ecosystem is currently gathered in tsdown.

CleanShot 2025-11-08 at 18 26 54@2x

@benjie

benjie commented Nov 9, 2025

Copy link
Copy Markdown
Member

I’m not sure we need to add tsup or tsdown as dependencies. More dependencies, more dependabot alerts 😅

@NShahri

NShahri commented Nov 12, 2025

Copy link
Copy Markdown
Author

Thanks @benjie @productdevbook
Switched to using Node’s type stripping for development and tsc for the build step.

@codecov

codecov Bot commented Nov 12, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a107730) to head (9f96722).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #385   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          164       167    +3     
  Branches        52        55    +3     
=========================================
+ Hits           164       167    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread package.json Outdated
"module": "dist/index.js",
"typings": "dist/index.d.ts",
"main": "dist/cjs/index.js",
"module": "dist/esm/index.js",

@productdevbook productdevbook Nov 12, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"main": "dist/index.js",
"module": "dist/index.mjs",

Can we extract this as mjs?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @productdevbook

Unfortunately, TypeScript doesn’t provide a built-in way to enforce specific file extensions, so implemented a custom postbuild script to handle that.

This is one of the advantages of using a tool like tsdown, as it abstracts these details and simplifies the build process.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

main and module are outdated in favor of exports ("package entry points") which wouldn't require a postbuild step.

@pawelkrystkiewicz

Copy link
Copy Markdown

Hey @NShahri, are you still working on this feature?

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.

6 participants