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

Jump to bottom

Node 12 support #288

Merged
merged 6 commits into from
May 9, 2019
Merged

Node 12 support #288

merged 6 commits into from
May 9, 2019

Conversation

Eugeny
Copy link
Contributor

@Eugeny Eugeny commented May 4, 2019 •
edited
Loading

fixes #279

@msftclas
Copy link

msftclas commented May 4, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@zxdong262 zxdong262 mentioned this pull request May 5, 2019
Closed
@hieuhlc
Copy link

hieuhlc commented May 7, 2019

@Eugeny I tried to install node-pty from your branch but still got warnings and errors. My env is:

  • NodeJS: v12.1.0
  • Electron: v5.0.0
  • MacOS: v10.14.4

I really want this PR to be merged. Thank you for your work!

@Eugeny
Copy link
Contributor Author

Eugeny commented May 7, 2019 •
edited
Loading

@hieuhlc fixed - thanks for the feedback

@hieuhlc
Copy link

hieuhlc commented May 7, 2019

@Eugeny thank you, it works now!

@oznu
Copy link

oznu commented May 7, 2019

Nice work @Eugeny.

I've tested this also. Builds now work on macOS and Linux (Node v8->12; Electron v2->5) and Windows (Node v10->12; Electron v4->5).

However tests are failing on macOS with Node v12, they pass on Node v10 - I haven't tried the tests on other platforms.

@Eugeny
Copy link
Contributor Author

Eugeny commented May 8, 2019 •
edited
Loading

@oznu fixed!

@Tyriar Tyriar added this to the 0.9.0 milestone May 9, 2019
@Tyriar Tyriar self-assigned this May 9, 2019
Tyriar
Tyriar approved these changes May 9, 2019
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks @Eugeny. The change in unixTerminal.ts looks a little scary and I've been dreading needing to change that but the PR seems to work on Windows, Linux and macOS onb oth node 10 and 12 👍

@Tyriar Tyriar merged commit 929b04b into microsoft:master May 9, 2019
@Eugeny
Copy link
Contributor Author

Eugeny commented May 10, 2019

Brilliant - could you push out a new beta to NPM please?

@Eugeny Eugeny deleted the node12 branch May 10, 2019 10:20
@Tyriar
Copy link
Member

Tyriar commented May 10, 2019 •
edited
Loading

@Eugeny 👌 just set up automatic beta releases just like on xterm.js #294

@Eugeny
Copy link
Contributor Author

Eugeny commented May 10, 2019

Thanks - will be in the next Terminus build

@exsilium exsilium mentioned this pull request May 16, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Feb 18, 2020
Closed
This was referenced Mar 15, 2020
Open
Open
@snyk-bot snyk-bot mentioned this pull request Apr 7, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2020
Open
@alexandre-cezar alexandre-cezar mentioned this pull request Jul 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tyriar Tyriar Tyriar approved these changes

Assignees

@Tyriar Tyriar

Labels
None yet
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.

node-pty doesn't build on Node 12 (V8 7.4)
5 participants
@Eugeny @msftclas @hieuhlc @oznu @Tyriar