Skip to content
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

cd path does not work as expected #153

Closed
mxstbr opened this issue Jul 24, 2016 · 6 comments
Closed

cd path does not work as expected #153

mxstbr opened this issue Jul 24, 2016 · 6 comments

Comments

@mxstbr
Copy link
Contributor

mxstbr commented Jul 24, 2016

When having created a new app, create-react-app outputs this information:

Success! Created weather-app at /Users/a2517/sites/commercial/plotly/academy/weather-app.

…

We suggest that you begin by typing:
  cd /Users/a2517/sites/commercial/plotly/academy/weather-app
  npm start

screen shot 2016-07-24 at 10 08 14

As you can see, there's two absolute paths there, even though it doesn't really make sense to have the cd path be absolute. Instead, the instructions should show:

cd weather-app
npm start

Looking at the source of this, it seems like there was some sort of precaution taken to only show the absolute path when one is no longer in the original folder, but it's not working as expected since it shows the absolute path even when one is in the original folder:

// init.js:58

// Make sure to display the right way to cd
    var cdpath;
    if (path.join(process.cwd(), appName) === hostPath) {
      cdpath = appName;
    } else {
      cdpath = hostPath;
    }

(hostPath is absolute, it should only show the appName when still in the original folder, as in my case)

Something about this check is off: path.join(process.cwd(), appName) === hostPath. I'm not well versed enough in that part of the codebase to find the bug in this code, but maybe @vjeux or @gaearon have some idea?

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

cc @lacker

@lacker lacker self-assigned this Jul 24, 2016
@lacker
Copy link
Contributor

lacker commented Jul 24, 2016

Oops. Looks like there is a process.chdir() happening in the global-cli before that check happens, so process.cwd() is no longer the original working directory and that check is always returning false. Let me see if there is some way to access the original working directory without changing the global-cli which is fidgety....

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 24, 2016

It's not like this is a major bug in the global CLI, people can use it just fine as-is. With #152 merged we already have some changes there we have to release anyway.

If there's no way to get a clean, simple solution for this outside the CLI I'd rather fix this is a new version of the CLI than hack around the issue with a bunch of fragile lines of code someplace else.

@lacker
Copy link
Contributor

lacker commented Jul 24, 2016

Makes sense. I just need to be sure that "old CLI" and "new react-scripts" work together. So if there's something like passing an extra argument into the init script it will have to still behave reasonably if the new argument is undefined.

@vjeux
Copy link
Contributor

vjeux commented Jul 24, 2016

If you don't know about pushd and popd, this may be useful here

http://unix.stackexchange.com/questions/77077/how-do-i-use-pushd-and-popd-commands

@lacker
Copy link
Contributor

lacker commented Jul 25, 2016

#166 fixes this.

I just changed the global-cli. don't think this should use pushd and popd because we want to be Windows-compatible. So it's better to only interact with the shell via the abstractions that Node provides. That's easier to code anyway ;-)

@lacker lacker closed this as completed Jul 25, 2016
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants