https://pantsbuild.org/ logo
Join Slack
Powered by
Hey folks, hoping to get some guidance from people...
# development
b
Hey folks, hoping to get some guidance from people who understand and work with either the JavaScript backend or NodeJS tools; I was looking to update the version of Pyright (which is a NodeJS tool) - and I see that the default configuration still uses Node 16, which is now deprecated. Is there any reason not to upgrade the default Node version for tools to be something newer / supported?
w
Unless @worried-painter-31382 says otherwise, it should be fine to update the defaults to maybe 20.14.x LTS? When I wrote that plugin, 16 was the LTS - and it's never been updated I guess?
Or honestly, maybe even 22.x? CI might need some tweaks, in the event we specify the node version there as well.
And ditto for 
example-javascript
b
NodeJS active version is up to 20 now, I would prefer to update to 22 if possible because that becomes the active version within a few months (looks like before EOY)
w
I'm fine with it - so long as all the tests still pass, 🤷
Agree that we should get off node 16, generally though
b
I’m also hoping that upgrading Node + Pyright could result in some significant performance improvements; Pyright is one of the slower parts of our pipeline right now
w
... cough.... I didn't already have a conversation about this and forget about it... Nope, not me - that's for sure https://pantsbuild.slack.com/archives/C046T6T9U/p1714283582806289
b
Oh wow, thanks; this is almost exactly my problem 🙂
w
... cough again... nope, didn't forget to update Node in Pants, nope, definitely not me
b
And yes, I figured out how to change the NodeJS version locally and it does yield a pretty significant perf improvement
Anyhow, no worries on forgetting and I’m happy to make this upgrade and send a PR
w
😄 Also, if you're PRing, maybe take a read through the comments here? Just to check if Tobias made any comments about Node 16 specifically, or if it was related to corepack and stuff https://github.com/pantsbuild/pants/pull/20826
it does yield a pretty significant perf improvement
Any numbers? Something worthy of the release notes on a PR?
b
Actually yeah, pretty solid. Here’s a before / after: Before (v16.15.0)
Copy code
pants --no-local-cache --no-pantsd check ::

16:32:19.64 [INFO] Completed: Force venv to materialize
16:32:41.98 [INFO] Completed: Run Pyright on 442 files.
(so about 22.34 seconds to completion) After (v22.2.0)
Copy code
pants --no-local-cache --no-pantsd check ::

16:33:12.89 [INFO] Completed: Force venv to materialize
16:33:29.00 [INFO] Completed: Run Pyright on 442 files.
(16.11 seconds to completion) This is about a 28.9% improvement! I used 
--no-local-cache
and 
--no-pantsd
to hopefully ensure that caching does not influence the results.
w
Not bad, in that linked thread, it was like, 10 seconds for 5 files or something
b
Yeah, IDK what to make of that. There’s definitely some startup latency to the whole thing because running Pyright on a single file in my own repo takes ~3.7 seconds (even with the new Node version).
Copy code
16:39:55.47 [INFO] Completed: Force venv to materialize
16:39:59.14 [INFO] Completed: Run Pyright on 1 file.
w
Once I can get some free cycles back, my plan is hard focus on perf and startup time - I feel like a frog got boiled at some point.
b
It’s also possible that simply upgrading both Node and Pyright might help; they’re pretty significantly out of date 🤷 That’s my next project after upgrading the Node version, upgrading Pyright as well
w
🎉
w
NodeJS upgrade has some implications because it also upgrades the version of corepack -> it updates the default version of NPM for all users. See https://github.com/pantsbuild/pants/pull/20617 and https://github.com/pantsbuild/pants/pull/20826/commits/0c0f47e4534b404a147452add8dec3cedcfcafd9 for attempts. I've reviewd both but neither are changes I'd be willing to accept as is.
Any approach to this upgrade should go from 16 -> 18 is my advice, before attempting 20, for the same reason any software should be carefully migrated along major versions.
b
Thanks for this - do you have some notion of what you wouldn’t be willing to accept from these attempts / what is missing from them?
w
Mostly because the tests are failing 😄 I didn't mean to sound negative towards the contributions, I just think the change needs to be applied 1. In isoliation 2. one step at a time.
Meaning going from 16 to 20 is too large a change for me to feel confident in being a stable migration, and doing the nodejs upgrade as part of a another merge request is asking for trouble.
But upgrading the default is long overdue, as you noted!
b
Sure, I think all of that feedback is totally reasonable and going 1 even version at a time seems healthy to ensure that we know where the regressions come from
w
My only thought is that, going from Node 16 to 18 is a defaults change, so unless the thought is that the defaults are unstable? I actually don't know if Pants has a policy about default updates now that I think about it
doing the nodejs upgrade as part of a another merge request is asking for trouble
💯 !!!
Note: I have no opposition, or preference, to going to 18, 20, or 22 - all fine by me - I think in the release notes, we should just explicitly call out how users can update to something else on their own.
n
One issue I noticed in attempting this recently is that the default package manager versions also get bumped automatically - this requires changes to lockfiles in tests across the backend. Also from memory, yarn goes from v1 -> v4, which is a big change.
b
Right, I believe that's the same problem that Tobias points out w.r.t. npm and corepack; and the surface area could definitely be large so I'll see what issues I run into
👍 1
n
@better-van-82973 Here are the latest known_version strings if it saves you some time at least for that bit.
🙏 1
w
Ah right, 
pnpm
and whatnot automatically backfill into package.json - that's been a grief recently. I've really gotta get working on that version migration goal - this would be a great case where a user who upgrades automatically gets node 16 put in the 
pants.toml
if nothing else is specified, ensuring updates don't break anything
b
I split out upgrading Pyright into a separate PR: https://github.com/pantsbuild/pants/pull/21007 After that I’ll make another one to upgrade Node to 18
w
I've opened https://github.com/pantsbuild/pants/pull/21021 for merge, I think getting this in is a good idea so that package manager versions are predicable in tooling. There's currently tests that are affected by this bug that could fail by e.g upgrade node.
🎉 2
b
Indeed, I already found that out the hard way: https://github.com/pantsbuild/pants/pull/21012 Looking forward to your corepack PR landing, I think it’ll be a huge upgrade
8 Views
Open in Slack
PreviousNext
Powered by