-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Create a node.js target for xterm.js #3212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, some high level comments below 🙂
src/common/public/tsconfig.json
Outdated
"include": [ | ||
"../**/*", | ||
"../../../typings/xterm-core.d.ts", | ||
"../../../typings/xterm.d.ts", // common/Types.d.ts imports from 'xterm' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a problem, we can't import from xterm if we want it to run for node because it touches DOM types which will throw for tsconfig targets without the dom lib.
typings/xterm-core.d.ts
Outdated
* to be stable and consumed by external programs. | ||
*/ | ||
|
||
declare module 'xterm-core' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another problem I'd like for us to solve is the sharing of these typings, I think having xterm.d.ts
import from xterm-core.d.ts
will cause problems because of this module declaration. We may need separate .d.ts that just export the core types without the module wrapping them to be able to share.
This reverts commit f685d8e.
Hey @Tyriar, here's an attempt at what you described in #2749 (comment). This PR:
src/browser/public/Terminal.ts
-->src/common/public/Terminal.ts
.src/browser/Terminal.ts
-->src/common/public/TerminalCore.ts
sincesrc/browser/public/Terminal.ts
wrapssrc/browser/Terminal.ts
.src/browser
imports fromsrc/common/public/Terminal.ts
andsrc/common/public/TerminalCore.ts
. I took a guess at what to remove based on what I thought would still be meaningful in the Node environment and on the discussion in Create a node.js target for xterm.js #2749, but I'd appreciate feedback here.node-test/index.js
, which I used to verify that the changes described above can be compiled, built, and used in a Node project based on the included README.src/common/public/Terminal.ts
. Changes to that file should be disregarded for now.I'm expecting this to need lots of work—looking forward to your feedback!