phoenix icon indicating copy to clipboard operation
phoenix copied to clipboard

node-pty dependency breaks running Phoenix in browser

Open AtkinsSJ opened this issue 1 year ago • 0 comments

Running Phoenix in the browser now fails because of the node-pty dependency added for the PathCommandProvider. This causes us to include a binary file, which the rollup common-js plugin gets very upset about.

We don't need or want that included, since PathCommandProvider only works when running in the Node CLI. However, excluding source files from rollup is less straightforward than I'd hoped and I'm not sure what I'm doing. :sweat_smile:

We can prevent the code including that with this change:

diff --git a/src/puter-shell/main.js b/src/puter-shell/main.js
index 9c2715c..3898be2 100644
--- a/src/puter-shell/main.js
+++ b/src/puter-shell/main.js
@@ -27,7 +27,6 @@ import { Context } from "contextlink";
 import { SHELL_VERSIONS } from "../meta/versions.js";
 import { PuterShellParser } from "../ansi-shell/parsing/PuterShellParser.js";
 import { BuiltinCommandProvider } from "./providers/BuiltinCommandProvider.js";
-import { PathCommandProvider } from "./providers/PathCommandProvider.js";
 import { CreateChatHistoryPlugin } from './plugins/ChatHistoryPlugin.js';
 import { Pipe } from '../ansi-shell/pipeline/Pipe.js';
 import { Coupler } from '../ansi-shell/pipeline/Coupler.js';
@@ -36,6 +35,12 @@ import { MultiWriter } from '../ansi-shell/ioutil/MultiWriter.js';
 import { CompositeCommandProvider } from './providers/CompositeCommandProvider.js';
 import { ScriptCommandProvider } from './providers/ScriptCommandProvider.js';
 
+// PathCommandProvider is only compatible with node.js for now
+let PathCommandProvider;
+if (typeof window === 'undefined') {
+    PathCommandProvider = await import('./providers/PathCommandProvider.js');
+}
+
 const argparser_registry = {
     [SimpleArgParser.name]: SimpleArgParser
 };

...however, rollup can't tell that this (or whatever other check) doesn't include PathCommandProvider.js, so it adds it anyway.

A temporary workaround is to not include it at all:

diff --git a/src/puter-shell/main.js b/src/puter-shell/main.js
index 9c2715c..9cb6d9b 100644
--- a/src/puter-shell/main.js
+++ b/src/puter-shell/main.js
@@ -27,7 +27,6 @@ import { Context } from "contextlink";
 import { SHELL_VERSIONS } from "../meta/versions.js";
 import { PuterShellParser } from "../ansi-shell/parsing/PuterShellParser.js";
 import { BuiltinCommandProvider } from "./providers/BuiltinCommandProvider.js";
-import { PathCommandProvider } from "./providers/PathCommandProvider.js";
 import { CreateChatHistoryPlugin } from './plugins/ChatHistoryPlugin.js';
 import { Pipe } from '../ansi-shell/pipeline/Pipe.js';
 import { Coupler } from '../ansi-shell/pipeline/Coupler.js';
@@ -36,6 +35,9 @@ import { MultiWriter } from '../ansi-shell/ioutil/MultiWriter.js';
 import { CompositeCommandProvider } from './providers/CompositeCommandProvider.js';
 import { ScriptCommandProvider } from './providers/ScriptCommandProvider.js';
 
+// PathCommandProvider is only compatible with node.js for now
+let PathCommandProvider;
+
 const argparser_registry = {
     [SimpleArgParser.name]: SimpleArgParser
 };

AtkinsSJ avatar Apr 11 '24 12:04 AtkinsSJ