-
Notifications
You must be signed in to change notification settings - Fork 521
Port DataLoader to TypeScript #385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…cript and tsup, remove flowbin and babel
…hod as private method in DataLoader
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.
Amazing
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.
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 :/ |
| global._onUnhandledRejection = handler => { | ||
| _.on('unhandledRejection', handler); | ||
| }; | ||
|
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still failing unfortunately.
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.
- 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?
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.