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

eject removes linked react-scripts copy instead of symlink #1732

Closed
tuchk4 opened this issue Mar 6, 2017 · 8 comments
Closed

eject removes linked react-scripts copy instead of symlink #1732

tuchk4 opened this issue Mar 6, 2017 · 8 comments
Milestone

Comments

@tuchk4
Copy link
Contributor

tuchk4 commented Mar 6, 2017

Starts here #1356 (comment)

ownPath - path to react-scripts root.

If react-scripts is linked - app node_modules looks like this

App/
  |- node_modules/
                 |- react-scripts/ -> (symlink)/lib/node_modules/react-scripts
Case ownPath Notes
resolveApp('node_modules/react-scripts') APP/node_modules/react-scripts Path to react-scripts symlink
resolveOwn('.') CRA_DIR/packages/react-scripts Path to local react-scripts copy
resolveOwn('react-scripts') LOCAL_CRA/packages/react-scripts/react-scripts Path is wrong

1st and 2nd cases are the same expect eject - we remove (eject.js:160) react-scripts from node_modules after eject.
In 2nd case local CRA packages/react-scripts will be removed instead of app node_modules/react-scripts.

@Timer We opted to switch to resolveOwn('.') because we don't want to hardcode the package name to be react-scripts.


I had unpleasant situation when run eject at app with linked react-scripts and my local CRA packages/react-scripts were removed with uncommitted changes :(

I have another implementation without name hardcode. I will test it and make new PR

@tuchk4
Copy link
Contributor Author

tuchk4 commented Mar 6, 2017

btw. Also hardcode at check if react-scripts is linked at paths.js#L116

@tuchk4
Copy link
Contributor Author

tuchk4 commented Mar 6, 2017

Suggested implementation:

1. Read name from package.json

const { name } = require('../package.json');
resolveApp(`node_modules/${name}`);

const reactScriptsPath = path.resolve(`node_modules/${name}`);
const reactScriptsLinked = fs.existsSync(reactScriptsPath) && fs.lstatSync(reactScriptsPath).isSymbolicLink();

and even save node_modules/${name} to variable.

2. Add additional check at eject script

wrap eject.js:160 with such if:

if (ownPath.indexOf(appPath) === 0) { 
 // remove react-scripts and react-scripts binaries from app node_modules
}

This check should work because for not linked react-scripts:

expect(ownPath).toEqual(`${appPath}/node_modules/react-scripts`);

1st way is more clear for me.

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2017

Why do you not like the second option? "Don't destroy what isn't yours" seems like a generally useful maxim to contain damage in case of other potential bugs in the future.

@gaearon gaearon added this to the 0.9.4 milestone Mar 6, 2017
@tuchk4
Copy link
Contributor Author

tuchk4 commented Mar 6, 2017

It is more clear for me because - in this way ownPath has the correct path relative to the application. And it is the same for both cases (linked react-scripts or not) . So should be expected behaviour for all scripts that use ownPath. And I don't want scripts to be dependent on if linked react-scripts or not except paths.js

"Don't destroy what isn't yours"

Sounds very reasonable but if (ownPath.indexOf(appPath) === 0) { may make code more complex and confuse new contributors.


If you vote for 2nd way, then I suggest little update:

wrap eject.js:160 with such if:

if (ownPath.indexOf(appPath) === 0) { 
 // remove react-scripts and react-scripts binaries from app node_modules
}

and read name from package.json to remove name hardcode at check if react-scripts is linked at paths.js:116

const { name } = require('../package.json');

const reactScriptsPath = path.resolve(`node_modules/${name}`);
const reactScriptsLinked = fs.existsSync(reactScriptsPath) && fs.lstatSync(reactScriptsPath).isSymbolicLink();

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2017

If you vote for 2nd way, then I suggest little update

Yep, let's do it like this.

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2017

@tuchk4 Do you want to send a PR for this?

@tuchk4
Copy link
Contributor Author

tuchk4 commented Mar 6, 2017

@gaearon yeap.

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2017

Fixed in #1736.

@gaearon gaearon closed this as completed Mar 6, 2017
@lock lock bot locked and limited conversation to collaborators Jan 22, 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

2 participants