-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
fix: support development on Node 20 #725
Conversation
This patches node-fetch so pnpm install works on Node 20: pnpm/pnpm#6424
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.
The fix for Node 20 is much appreciated, but as far as I can tell that only requires updating pnpm
.
The rest seem to functionally change what the CI process is doing to not align with what I had originally intended, so I'm a little unclear as to the motivation for these changes.
If you could clarify some of that for me, it would be super helpful!
This reverts commit 2246980.
I originally planned on just updating pnpm, but I wanted to make sure it would be properly tested in CI so I could work on #726, so I was a bit over-cautious since I couldn't start the CI builds myself. Sorry for any confusion/noise! |
@nickmccurdy No problem! If you want to just update this to include the |
This reverts commit cf6f507.
This comment was marked as resolved.
This comment was marked as resolved.
Right now the only change I understand the motivation for is the Definitely appreciate your help and looking forward to figuring out the best way to go about some of this! I'm sure there are improvements that could be made |
The build is failing on The remaining changes in this pr we haven't already discussed:
|
I'm not sure how the formatting got out of sync that sometimes causes diffs like the one in this PR for My guess is that it has to do with the docgen process that occurs in CI when a new version is released. Maybe somehow after that runs not everything is properly reformatted? |
I updated the repo's settings to run workflows on a user's first PR for the repo, so hopefully that will resolve the issue (previously I was being prompted to manually approve them for this PR). |
I extracted |
Huh, so you ran format locally, got this result and pushed it, then it fails the CI check? What happens when you run format then check format locally? |
Yes, though |
Now the format of |
@nickmccurdy 😵 I mean, the formatting issue should be relatively minor and if someone does end up pushing incorrectly formatted code, if it's really bad, it should be apparent during PR, and if it's minor, it will just be fixed as soon as anyone else merges code, so I'm not super worried about that if it's a hassle. I'm watching the issue you mentioned , definitely seems pretty critical for them to address so hopefully will be resolved soon. It's honestly just too much for me to add all those super verbose |
I think it's important to still fix this in the interim, as you basically can't develop locally with the latest stable version of Node until this is merged. ts-node is also known to lag behind in support. If you want to eliminate tech debt, it would probably be simpler to just replace it with a more modern compiler. |
@nickmccurdy Yeah I buy that, would you be willing to replace it with one of the more modern options? I'm honestly not particularly invested in how our TS is executed during dev as long as it works. |
@nickmccurdy Hey trying to get the Node 20 stuff integrated with some test changes we just merged. It's honestly been really rough unfortunately tons of rough edges with the breaking changes to loaders. Had issue with both ts-node and tsx. Anyways, I'll wrap up this PR now that there's a bunch of crazy changes on it and I have the context needed, so thanks so much for your help and hopefully the next node release will be smoother 🙏 |
@nickmccurdy Welp I've spent many hours on this and it's just problem after problem with Node 20. For now I'm going to give up on this and merge some of the changes I've made and disable Node 20 in CI 😔 If you have any more ideas about how to resolve this stuff after I merge, please feel free to take a stab at it: |
This patches node-fetch so pnpm install works on Node 20: pnpm/pnpm#6424
I also added some updates and CI fixes.