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

Bi directional communication #100

Merged
merged 4 commits into from
Jul 24, 2018
Merged

Bi directional communication #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ee2046
Use 'child.stdin.write' to send 'eject' command
joshwcomeau Jul 22, 2018
d2dc9c9
Fix bug where app would crash after ejecting with modal open
joshwcomeau Jul 22, 2018
b15fc38
Cleanup
joshwcomeau Jul 22, 2018
9c6c0a1
Improve fix to use getDerivedStateFromProps
joshwcomeau Jul 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/components/TaskRunnerPane/TaskRunnerPane.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,21 @@ class TaskRunnerPane extends Component<Props, State> {
selectedTaskId: null,
};

static getDerivedStateFromProps(props, state) {
// It's possible that this task is deleted while the modal is open;
// For example, This can happen when ejecting the project, since the
// create-react-app "eject" task removes itself upon completion.
const selectedTaskExists = props.tasks.some(
task => task.id === state.selectedTaskId
);

if (!selectedTaskExists) {
return { selectedTaskId: null };
}

return null;
}

handleToggleTask = taskId => {
const { tasks, runTask, abortTask } = this.props;

Expand Down
30 changes: 21 additions & 9 deletions src/middlewares/task.middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,16 +132,8 @@ export default (store: any) => (next: any) => (action: any) => {
additionalArgs.push('--', '--coverage');
}

/* Bypasses 'Are you sure?' check when ejecting CRA
*
* TODO: add windows support
*/
const command =
project.type === 'create-react-app' && name === 'eject'
? 'echo yes | npm'
: 'npm';
const child = childProcess.spawn(
command,
'npm',
['run', name, ...additionalArgs],
{
cwd: projectPath,
Expand All @@ -158,6 +150,20 @@ export default (store: any) => (next: any) => (action: any) => {
next(attachTaskMetadata(task, child.pid));

child.stdout.on('data', data => {
// The 'eject' task prompts the user, to ask if they're sure.
// We can bypass this prompt, as our UI already has an alert that
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@superhawk610 superhawk610 Jul 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// confirmation copy changes?
const isEjectPrompt = data
.toString()
.includes('Are you sure you want to eject? This action is permanent');

if (isEjectPrompt) {
sendCommandToProcess(child, 'y');
}
next(receiveDataFromTaskExecution(task, data.toString()));
});

Expand Down Expand Up @@ -270,3 +276,9 @@ const getDevServerCommand = (
throw new Error('Unrecognized project type: ' + projectType);
}
};

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`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

@AWolf81 AWolf81 Aug 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, not sure what it was but after removing node_modules and reinstalling. Ejecting is working like it is.

};