Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Switch to node-pty #547

Closed
ashthespy opened this issue Jun 11, 2018 · 14 comments
Closed

Switch to node-pty #547

ashthespy opened this issue Jun 11, 2018 · 14 comments

Comments

@ashthespy
Copy link

Can we switch from the unmaintained pty.js to node-pty
To make life even easier - node-pty-prebuilt could be utilised thus making this package simpler to maintain. You wouldn't have to prebuild pty.js for each node and OS version as you do currently.

Can submit a PR (It's two lines ;-) ) if there is traction for the idea!

@the-j0k3r
Copy link
Collaborator

the-j0k3r commented Jun 16, 2018

Read #543

and then read #425

@the-j0k3r
Copy link
Collaborator

the-j0k3r commented Jun 16, 2018

HAHAHA @ivankravets a PR exists see #425

@ivankravets
Copy link
Member

@the-j0k3r what do you mean? Which PR?

@the-j0k3r
Copy link
Collaborator

the-j0k3r commented Jun 16, 2018

Seriously?

2nd post

Read #543

and then read #425

Third post

HAHAHA @ivankravets a PR exists see #425

Ive put it in two posts before, 3rd time lucky

#425 <- that

@ashthespy
Copy link
Author

@the-j0k3r it would be much easier to shift to node-pty-prebuilt
@ivankravets Travelling now - but will fork and submit a PR.

@ivankravets
Copy link
Member

@the-j0k3r do you understand how does node-pty work?

That is what I've said before to @Fred-Barclay, the one thing is to merge PR, and another is to review and understand what does code do.

We should be careful with all PRs, I see that over 1M downloads we have for the last time https://atom.io/packages/platformio-ide-terminal.

@the-j0k3r
Copy link
Collaborator

the-j0k3r commented Jun 16, 2018

Im not @Fred-Barclay, Im just pointing to the fact the PR is relevant to the question being raised.

If that PR isnt suitable its up to you to close it. Not leave things open for no reason. Anyway Im not your maintainer nor wish to be, Im already investing too much time answering Q's and letting ppl know whats going on.

@ivankravets
Copy link
Member

"node-pty" is a fork of https://github.com/chjj/pty.js and depends on a host compiler too. The main difference is that @Tyriar ported JavaScript part to Microsoft's TypeScript.

@the-j0k3r
Copy link
Collaborator

No explanation needed I know the issue with #425, but IDK if you understand what Im saying, clearly you dont.

Nite and congrats on release. Maybe future PR's wont be ignored forever.

@ivankravets
Copy link
Member

If that PR isnt suitable its up to you to close it.

I've just closed it.

@ashthespy You can try to switch to "node-pty", please don't forget to compile it on Linux, Windows, Mac. If it works, please make a PR. I think I can prepare binaries or we can do that together.

@ashthespy
Copy link
Author

@ivankravets That is the thing - you don't need to make binaries with node-pty-prebuilt

@the-j0k3r
Copy link
Collaborator

the-j0k3r commented Jun 16, 2018

@ivankravets That is the thing - you don't need to make binaries with node-pty-prebuilt

Yep.

I've just closed it.

Lovely :) now its looking like something is going on maintenance wise.

@ivankravets
Copy link
Member

@ashthespy I see, that package does the same what we do last years. The only problem is what to do when new Electron is out. Users will need to uninstall/install terminal again or we should do face incremental release to trigger "node-pty-prebuild" install script which will pull new module (if it is already precompiled.).

Nevertheless, please make a PR and restest it. I will also test it on a few VMs.

@the-j0k3r
Copy link
Collaborator

@ivankravets please close no longer relevant

@platformio platformio deleted a comment from zorn-v Jul 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants