search

facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.69k stars 26.84k forks source link

Should it be allowed paths on the --scripts-version argument? #4266

Closed mAiNiNfEcTiOn closed 6 years ago

mAiNiNfEcTiOn commented 6 years ago

At work I was creating our own react-scripts package, but to test it I needed to define a path rather than a package name.

So for that I ran create-react-app my-example-app --scripts-version file:/my/absolute/path

Obviously, this is a misuse of the attribute and I know this is not a bug, it's just how things are 😉

On the other hand, I noticed that the only reason why this didn't work was because in this line on createReactApp.js it is expected that packageName is a part of the path and not the path itself...

So in the beginning of this function I did a mutation of the argument (I know I know, bad boy...):

packageName = packageName.startsWith('file:') ? path.basename(packageName.substr(5)) : packageName;

I'd like to highlight that this serves only my purpose...

So my question is... Imagining that I'd refactor it to a proper fix, that would imply I detect (on the failing places) that a the packageName.startsWith('file:'), then I use as the start of the path.resolve() the packageName directly. Why? To cover cases where the packageName is something like /my/absolute/path/@org/react-scripts, where a basename would kill it 😛

Would you be interested in it?

mAiNiNfEcTiOn commented 6 years ago

Hmmm... maybe a more important question... is the --scripts-version the right param flag to use? I mean, the version part of it makes me doubt if it should be used for a path or even a different packageName.

iansu commented 6 years ago

You can already use file:... with --scripts-version. It's implemented here: https://github.com/facebook/create-react-app/blob/next/packages/create-react-app/createReactApp.js#L397

We also added some additional documentation about using file:... here: https://github.com/facebook/create-react-app/pull/4015

mAiNiNfEcTiOn commented 6 years ago

@iansu that is true with relative paths, because they can be resolved. What about absolute paths?

E.g.: file:/my/absolute/path/to/react-scripts?