Skip to content
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.

Sign up for GitHub

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

Upgrade node-pty to beta version #2420

Closed
wants to merge 4 commits into from
Closed

Upgrade node-pty to beta version #2420

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d3ba93
Upgrade electron-forge from 7.2.0 to 7.4.0 (#2399)
calcaide Jul 25, 2024
350d521
Icu 14449 migrate to @xterm (#2412)
calcaide Jul 25, 2024
0e5d73f
Upgrade node-pty to beta version
calcaide Jul 31, 2024
ddaa2a1
Merge branch 'main' into chore-ICU-14450-upgrade-node-pty-to-beta
calcaide Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ui/desktop/app/components/session/tabs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { inject as service } from '@ember/service';
import { Terminal } from 'xterm';
import { FitAddon } from 'xterm-addon-fit';
import { CanvasAddon } from 'xterm-addon-canvas';
import { Terminal } from '@xterm/xterm';
import { FitAddon } from '@xterm/addon-fit';
import { CanvasAddon } from '@xterm/addon-canvas';
import { v4 as uuidv4 } from 'uuid';
import debounce from 'lodash/debounce';

Expand Down
12 changes: 6 additions & 6 deletions ui/desktop/electron-app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,19 @@
"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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we upgrading to a beta version again?

Copy link
Collaborator Author

@calcaide calcaide Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Here link to the test run of this PR

Copy link
Collaborator

@ZedLi ZedLi Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@calcaide calcaide Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @xterm is 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 @xterm is publishing a stable version that contains node-pty beta 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 @xterm migration since without this node-pty upgrade we can not get successful runs on our pipeline.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@calcaide calcaide Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"semver": "^7.5.3",
"shell-env": "^3.0.1",
"shell-quote": "^1.7.3",
"tree-kill": "^1.2.2",
"which": "^4.0.0"
},
"devDependencies": {
"@electron-forge/cli": "^7.2.0",
"@electron-forge/maker-deb": "^7.2.0",
"@electron-forge/maker-dmg": "^7.2.0",
"@electron-forge/maker-squirrel": "^7.2.0",
"@electron-forge/maker-zip": "^7.2.0",
"@electron-forge/cli": "^7.4.0",
"@electron-forge/maker-deb": "^7.4.0",
"@electron-forge/maker-dmg": "^7.4.0",
"@electron-forge/maker-squirrel": "^7.4.0",
"@electron-forge/maker-zip": "^7.4.0",
"electron": "31.0.1",
"unzipper": "^0.11.5"
},
Expand Down
Loading