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.

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

Feature request: export React component #260

Closed
olalonde opened this issue Sep 2, 2016 · 15 comments
Closed

Feature request: export React component #260

olalonde opened this issue Sep 2, 2016 · 15 comments
Assignees
Labels
type/enhancement Features or improvements to existing features

Comments

@olalonde
Copy link

olalonde commented Sep 2, 2016

Would be really useful if xterm.js would export a React component for easily embedding in React apps. It could just be a simple wrapper around the existing code.

@parisk
Copy link
Contributor

parisk commented Sep 2, 2016

This is really a really interesting scenario. We try to keep xterm.js as self-contained and lightweight as possible, since it has already enough of functionality and complexity in it.

I think though that creating an xtermjs/xterm-react repository and an xterm-react npm module hat would just wrap xterm.js in a React component could do the job. How does that sound?

@olalonde
Copy link
Author

olalonde commented Sep 2, 2016

That would be great :)

@iMoses
Copy link

iMoses commented Sep 20, 2016

I would really like to work on that but I found a few issues with adjusting the code without making it too patchy. The problem is that xtem.js relays heavily on interactions with the DOM, so separating the view logic is extremely difficult.

I'd like to make a feature request to separate the logic and DOM handling in a way that will allow different components to take over rendering. I suggest something in the lines of extracting Terminal.UI from Terminal, which would be responsible for attaching events to the DOM, creating and updating elements, etc. Terminal would wait for Terminal.UI to trigger events, such as keypress and scroll, that way it would be decoupled from rendering completely. I think you started going in that direction with the Viewport, but I believe a full separation is in order.

I'd like to hear your thoughts on this, and I'd like to help with it in case you think it's relevant.

I believe that for the complexity of code in this project, some separation of concerns could really help in future development and maintenance, and I'm a big fan of the single responsibility principle 😄

@Tyriar
Copy link
Member

Tyriar commented Sep 21, 2016

@iMoses we've been slowly trying to modularise the code, the ability to move modules out of the main file was only just added. Here is a related issue which also calls for this separation: #266

Starting with viewport to keep the different manageable sounds good 👍

@parisk parisk added the feature label Sep 29, 2016
@parisk
Copy link
Contributor

parisk commented Sep 29, 2016

Continuing the discussion from #285 as it is more suitable I think.

@iMoses can you tell which methods of the core module make it difficult for you to implement the react component?

Maybe the best thing would be branch only these out in their own module(s).

@iMoses
Copy link

iMoses commented Sep 29, 2016

Not everything in this list is difficult to work with, for example if I don't use the open method then other problematic method won't be triggered, but I still believe they should be separated into a different module. Most of what I'll list here can be found in my pull-request in the Interface.js file.

Anything in Viewport and CompositionHelper, but that's obvious :)

From the xterm.js file these are the main methods which should be separated:
blur, focus, bind*, initGlobal, prepareCopiedTextForClipboard, insertRow, open, loadAddon, destroy, refresh, attachCustomKeydownHandler, keyDown, evaluateKeyEscapeSequence, keyPress, bell, cancel.

'bell' and 'application-mode' should trigger events instead of interacting with UI logic.

I believe this is most of it, but we do need to watch out for uses of UI element in the code (e.g. this.viewport and this.element should never be used directly by the core module.

Hope this help.

P.S.
As I said, I'm currently working on a Reach XTerm.js example, for which I stripped down the xterm library to only the essentials I need. I'll be finished within less than a week, I hope, and then I'll share my code with you to review.
It currently works great on my machine, with the following exceptions: I didn't attach mouse events yet, I have a minor rendering issue the I'll need to fix by rewriting parts of refresh dur to the fact that I want React to handle rendering logic and not the library.

Hope this helps

@Tyriar
Copy link
Member

Tyriar commented Sep 29, 2016

It might be good to look into #266 before trying to tackle this

@iMoses
Copy link

iMoses commented Sep 29, 2016

If we separate between the core logic and the UI logic, the core can be initialized without worrying about the dom and only when needed the view can be initialized, passing it a terminal core instance to work with.

@iMoses
Copy link

iMoses commented Oct 1, 2016

I'm trying a test the mouse events to see that I didn't break anything, and I can't find a terminal application with full mouse support with includes mouse move, for example. What are you guys are testing this library against to verify that mouse events are working properly?

@parisk parisk mentioned this issue Aug 3, 2017
13 tasks
@farfromrefug
Copy link

I actually created an npm package for a react-xterm component.
You can start from there.
I can even transfer the github source project if wanted
https://github.com/farfromrefug/react-xterm

@parisk
Copy link
Contributor

parisk commented Aug 3, 2017

Thanks @farfromrefug! It would be great if we didn't start with "tabula rasa".

@parisk parisk added this to the 3.0.0 milestone Aug 3, 2017
@parisk parisk self-assigned this Aug 3, 2017
@parisk parisk removed this from the 3.0.0 milestone Nov 7, 2017
@Tyriar Tyriar added type/enhancement Features or improvements to existing features and removed type/feature labels Apr 4, 2018
@Tyriar
Copy link
Member

Tyriar commented Jun 1, 2018

This is something that would be better done by the community imo, I'm closing off to reduce our issue count but I encourage someone to put this together.

@Tyriar Tyriar closed this as completed Jun 1, 2018
@Tyriar Tyriar added the stale label Jun 1, 2018
@alex-saunders
Copy link

alex-saunders commented May 11, 2019

Hope you don't mind me commenting on a closed issue but I've been working on a custom React renderer to work with xtermjs. It provides a number of elements (such as <text>, <line>, <br>) that can be used to write to the terminal output. To use it, the package exports a render(element, HTMLElement) method that renders a terminal to a provided DOM element. It also works with existing React-DOM projects by providing a <Terminal> component that can be dropped in any existing components render method. Not sure if you want to do anything with this project but just thought i'd highlight it here incase anyone is still wanting a react integration:
https://github.com/alex-saunders/xterm-react-renderer

(still very much a WIP but it works as a proof of concept at the moment)

@Tyriar
Copy link
Member

Tyriar commented May 11, 2019

@alex-saunders thanks for calling it out, good to have a link in this issue in case people are looking 👌

@loopmode
Copy link

loopmode commented Nov 2, 2019

Hi. I was able to set up a react component with almost no code and using the "new" hooks system. However, it is rather tightly coupled with node-pty for usage in electron scenarios. I can't share any code right now, as it's an closed, internal project, but I encourage everyone to take a new shot at the problem using a hook-based solution - it's worth it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Features or improvements to existing features
Projects
None yet
Development

No branches or pull requests

7 participants