Bi directional communication #100
Conversation
| // confirms this action. | ||
| // TODO: Eject deserves its own Redux action, to avoid cluttering up | ||
| // this generic "RUN_TASK" action. | ||
| // TODO: Is there a way to "future-proof" this, in case the CRA |
There was a problem hiding this comment.
I checked react-scripts's eject code and seems like the prompted question is hard-coded and there is not any parameters for passing prompts (something like --force or --yestoall). I don't think it is something to change by its own. If there is going to be a change I think it would effect multiple places so I don't think we should worry about it for now.
There was a problem hiding this comment.
The discussion about Next.js support in #59, as well as this issue of future-proofing, makes me think it may be worthwhile to extract each framework's specific interactions into some sort of plugin file that would abstract CLI interactions into a consistent and predictable style - here's a very loose idea of what the structure might look like:
// plugins/create-react-app.plugin.js
const EJECT_CONFIRM_PROMPT = 'Are you sure you want to eject? This action is permanent';
const CMD_INSTALL = (path: string) => `npx create-react-app ${path}`;
const CMD_EJECT = () => `npx create-react-app eject`;
/*...*/
export const create = () => { /*...*/ };
export const eject = () => { /*...*/ };I imagine each plugin would have a create block and then could optionally have any number of framework-specific blocks as well, maybe exported as an array so they could be iterated over by the UI without any knowledge of how many there are in total?
Additionally, the CMD_ functions could all be piped through @bennygenel's formatCommandForPlatform.
This is clearly far too much boilerplate to be justified for just CRA and Gatsby, but as the project grows it has the marked benefit of placing all the CLI interaction logic in an easy-to-find (and thus easy to maintain) location and adds a further degree of separation between UI and business logic.
Yeah, I checked as well; might be worth opening an issue in CRA eventually. But not yet, it's not a real problem yet.
Yeah, totally agree. I think the proposal you made makes sense. I like the idea of a platform-specific util (like So the abstraction is not 100% clear to me yet and I suspect it may require some iteration... but I think it'll become critical as the project matures. |
|
@bennygenel @superhawk610 does this look ok? Could you approve the review, or request changes if there's stuff I should change? |
|
Thanks y'all! |
| const sendCommandToProcess = (child: any, command: string) => { | ||
| // Commands have to be suffixed with '\n' to signal that the command is | ||
| // ready to be sent. Same as a regular command + hitting the enter key. | ||
| child.stdin.write(`${command}\n`); |
There was a problem hiding this comment.
@superhawk610 At the moment, I'm merging windows-support & npm-to-yarn branch (not pushed yet) and there I've noticed that ejecting is not working with-out error message and it seems that it is happening at line 283. The error message is like in this issue.
Telling that the socket is already closed. Not exactly sure what happens here but it could be related that we're not sending end after writing to stdin.
Sending end can also be problematic because it could close the process.
I'll try to eject at Bennys windows-support branch so I know if it was working there. Maybe this could also be related to my setup:
Windows 10
npm -v 5.6.0
node -v v8.11.1
The app is ejected afterwards but there is this error message in console.
There was a problem hiding this comment.
OK, not sure what it was but after removing node_modules and reinstalling. Ejecting is working like it is.
Thanks for your contribution!
Please fill out the following template with details about your pull request:
Related Issue
#25
Summary
Thanks to @superhawk610's great suggestion, we no longer have to rely on
echoto send a command to the running process. Yay unblocking windows support!Incidentally, fixing this issue exposed a bug, which is that Guppy crashes when ejecting a project while the "View Details" modal is open. This is because the modal assumes a
taskwill be provided, but the eject task removes itself.The fix is simply to unset the
selectedTaskIdstate when the task is deleted. This feels like maybe it should move to Redux, since deriving state from props like this is discouraged... but I'm happy enough with this solution for now.