search

joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Remove node_modules before install after eject #331

Closed melanieseltzer closed 5 years ago

melanieseltzer commented 5 years ago

Related PR:

318

Summary:

As discussed in the chat, have to remove node_modules after eject and before install, because it was not sensing there was anything to install ("Already up to date" output) and subsequently getting the previous dev server failure from missing packages.

melanieseltzer commented 5 years ago

@AWolf81 Interesting, do you mean extract that output to display on screen upon eject? I suppose it could be pulled into a toast notification of some sorts. Do you know if there's a way to parse output and extract links to make them clickable? I think we should only show it somewhere if links can be clickable, since people are not going to want to copy/paste into the address bar.

Also, brings up something related, I wonder if we could make the links in terminal outputs clickable. I love being able to click on a link right there (e.g. open the dev server in the browser).

For the other thing, that sounds like a bug because the whole Eject task module is supposed to disappear afterwards. Is it consistent for you even after doing a reset state etc?

AWolf81 commented 5 years ago

@melanieseltzer I wouldn't parse the string from output as I think it would be OK to add a copy into a notification. We can add this once the save flash is available.

The idea with the clickable links is good but I think for every interaction we should render something in the UI but if we're not having anything in the UI it's OK to make links clickable. E.g. a linting error should open that file in editor. I think we can open an issue for this but we have to wait for eslint/flow plugin implementation so it will make sense to have clickable links in the terminal.

To the eject behaviour: This only happens after the dialog for unclean repo. On successful execution it will remove the eject row correctly. I haven't checked after a reset but I think it's the same.

melanieseltzer commented 5 years ago

Ah I see. So just create a custom message and show it in once eject is done? I think we should do it since sometimes you can miss the UI update once Eject is finished, so a success message would be useful.

Eject - whoops I misread your comment at first. Yeah I can reproduce that. Probably weirdness with the dialog, I'll take a look.

Side note are you experiencing the issue I logged in #332? Sounds like it if there's uncommitted changes.