Conversation
…cript and tsup, remove flowbin and babel
…hod as private method in DataLoader
ardatan
left a comment
There was a problem hiding this comment.
CI seems failing. Could you take a look?
|
Thanks @ardatan — I’ve fixed the CI issue. Could you please try re-running the workflow when you get a chance? |
|
@NShahri Still failing :/ |
jest.config.ts
Outdated
| global._onUnhandledRejection = handler => { | ||
| _.on('unhandledRejection', handler); | ||
| }; | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- Unfortunately, it wasn’t possible to add
unhandledRejectionhandler inJest, even when using customJest environments. Because of this limitation, I decided to switch to Vitest. - Additionally, I realized mocking
globaldependencies likeprocessorsetTimeoutwas 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.
There was a problem hiding this comment.
- Handling
unhandledRejectioninjestseems 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,
vitestdoes support handlingunhandledRejection, but it requires NodeJs 20+, which might limit compatibility. - Please correct me if I’m mistaken, but it looks like
unhandled.test.tsis 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?
|
tsup -> tsdown(https://tsdown.dev/) I definitely recommend using this. |
|
Wouldn’t it be better to use Node’s type stripping for development, tsc for build, and leave the bundling up to the consumer? |
|
I’m not sure we need to add tsup or tsdown as dependencies. More dependencies, more dependabot alerts 😅 |
|
Thanks @benjie @productdevbook |
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
package.json
Outdated
| "module": "dist/index.js", | ||
| "typings": "dist/index.d.ts", | ||
| "main": "dist/cjs/index.js", | ||
| "module": "dist/esm/index.js", |
There was a problem hiding this comment.
"main": "dist/index.js",
"module": "dist/index.mjs",
Can we extract this as mjs?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
main and module are outdated in favor of exports ("package entry points") which wouldn't require a postbuild step.

This PR migrates the entire codebase from JavaScript/Flow to TypeScript, modernizing the stack and improving type safety, tooling, and maintainability.
✅ Changes Included
npm📈 Benefits
This migration lays the foundation for better maintainability, improved tooling support, and enhanced type safety.