-
Notifications
You must be signed in to change notification settings - Fork 61
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
Support Node 18 + switch from ramda to rambda #326
Conversation
@lo1tuma, not be a bother, but can I get a review or update? |
Hi @taymoork2, sorry for the late reply, but I was on vacation the last two weeks. I hope to find some time this week to take a closer look at this PR and rambda itself. My first impression is it looks quite good and well maintained. Although it seems like they don’t support importing functions individually. Can you confirm this? Since we don’t use any bundle we won’t profit from thee-shaking. |
@lo1tuma it's all good
From what i can see, Rambda, does not support require('rambda/'). The screenshot above show's the runtime with individually imported functions/the PR, I tested using yalp (aka npm link) |
@taymoork2 I’ve also checked the result of the startup benchmark and it seems like I’ve only found one small issue, once that is fixed I’m happy to merge this PR. |
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.
LGTM. Thank you!
Thanks to @jrandolf for the original PR #325 this is based off