Upgrade node-pty to beta version #2420
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
calcaide
added
dependencies
| @@ -27,7 +27,7 @@ | |||
| "electron-log": "^5.1.2", | |||
| "electron-squirrel-startup": "^1.0.0", | |||
| "node-html-parser": "^5.3.3", | |||
| "node-pty": "^1.0.0", | |||
| "node-pty": "^1.1.0-beta14", | |||
There was a problem hiding this comment.
Why are we upgrading to a beta version again?
There was a problem hiding this comment.
@ZedLi Good question.
The 1.0.0 version is more than 1 year old and contains old versions of third-party libraries that contain deprecated code and have side effects that translate into a list of warnings went we are performing the DC build process (within the 3 OS's)
When we migrate from xterm to @xterm, we realize @xterm team was suffering some or even more issues than us with node-pty 1.0.0. They decided to move to node-pty beta version since 1.0.0 was blocking their build process due to the side effects mentioned.
Here their PR
Since we add node-pty as a dependency for xterm, and we migrate it, I thought following their steps will help us to not suffer side effects from old node-pty.
As demonstrate within screenshots on the description of this PR, Mac and Linux was a success eliminating the side effects, BUT in windows we start reproducing a new issue which we are already working with #tech-github-runners team to fix it. Here link to the slack thread
There was a problem hiding this comment.
Makes sense, do you know if the warnings were an issue at all? I think my concern is the risk of using a beta package since it hasn't been formally released nor has full support yet.
I also can't seem to find any changelogs for the beta releases, do you know if there any? I'm just curious what our risk is using the beta version. But it might not be that high if xterm is also using it since they're maintained in tandem?
There was a problem hiding this comment.
Your concerns are fair and happy to hold on this.
-
The warnings are a set of deprecated node methods node-pty is using. We are currently running node 18 on the pipeline as it is right now, are not an issue, just something to keep an eye but no issues that will stop the run. We have plans to upgrade the version of node running in the pipeline from 18 to 20. I will expect some of those deprecations happened within node 20, so this was a more preventive work. I have to mention that the windows runner warnings are more serious but still not breaking it 🤞
-
I do share the feeling of using beta libraries is risky, but in this specific case, I guess evaluate risks in both ends, keep using the old one or move to a beta one.
-
No, there is no changelog, neither any tags or public branches on their repo referring to what each beta version contains. I have been looking at their commit or PR history.
-
I decide to take the risk (not saying we have to) because we need to migrate xterm to
@xterm. During the migration process, I needed to also upgrade our node-pty to same version as the node-pty within@xterm. The main maintainer of@xtermis the main one in node-pty and on their release page, for the 5.4.0 they mention the upgrade to node-pty.
I though if@xtermis publishing a stable version that containsnode-ptybeta version, and the maintainers are the same people, the risk might be small, but I do recognize there is still a risk. -
I am fine if you think we should NOT do this, then I will hold on
@xtermmigration since without thisnode-ptyupgrade we can not get successful runs on our pipeline.
There was a problem hiding this comment.
I decide to take the risk (not saying we have to) because we need to migrate xterm to
@xterm. During the migration process, I needed to also upgrade our node-pty to same version as the node-pty within@xterm.
Does our node-pty versions need to match? What happens if we pull in different versions? If that's the case I'm fine with trying the beta package
There was a problem hiding this comment.
After some more testing, windows build shows errors and the build exists with errors.
I do see node-pty development is moving beta version up pretty quickly.
Since debugging this (beta version + windows runner) is a potential rabbit hole, I will hold on this upgrade until we get more capacity to spend time on the rabbit hole and perhaps node-pty moves from beta to stable soon.
|
Closing this PR until we get more capacity and can debug windows runner issues properly, more info within this comment |
Description
Update: Mac and linux runs are a success!!! old warnings are gone but windows run is failing. I am working with #tech-gihub-runners teams to help debug, it is possible we are missing a visualstudio individual component, as per node-pty readme
Upgrade node-pty to latest beta.
Notice that we declare
^1.1.0-beta14but1.1.0-beta7gets resolved. That is ok because we want the latest beta compatible with our third party deps that also use node-tpy, so1.1.0-beta7seems the safe one compatibility wise.How to Test
Screenshots
Old builds (build from 2.1.0 release) with
node-ptywarnings present in the 3 OS DC build process:Mac:
Windows:
Linux:
Test build of this branch (with node-pty, xterm and electron-forge upgrade):
Mac:
Linux:
Checklist