-
Notifications
You must be signed in to change notification settings - Fork 37
react-scripts > v2.1.1 don't have a webpack.config.dev #20
Conversation
webpack.config exposes config as a function which needs to be called with an env name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution again. Nice to see you back! 👋
We also need to update the logic to determine if the app is ejected or not, it is in the utils file. There was another PR opened that started to correct the same problem but it is pending for modifications from its author. However you can look there for the comments I made: #18 (comment)
Clean the version string before passing to semver, as ^ and ~ prefixes break the parsing
utils/index.js
Outdated
major: Number(semver.major(dependencies['react-scripts'])), | ||
minor: Number(semver.minor(dependencies['react-scripts'])), | ||
patch: Number(semver.patch(dependencies['react-scripts'])), | ||
major: Number(semver.major(reactScriptsVersionString)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semver.major only accepts a version string, not a ^ or ~ prefixed comparison string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, did you get an error with the previous behavior? I'd rather prefer that the prefix is taken into account because having ^1.3.0
does not mean that you have 1.3.0
installed hence you could have 1.3.7
. That's the reason I use the semver
utils for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes:
TypeError: Invalid Version: ^2.1.3
at new SemVer (/Users/johnsingleton/cra-test/my-app/node_modules/semver/semver.js:312:11)
I think because "^2.1.3" is a constraint not a version number. As is the code is just pulling whatever is in package.json, so I don't think it's getting the actual installed version anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you are right, there could always be a difference with the actual version installed but at least I feel we are a bit closer to the truth. 😄 I'll need to look a bit more in semver
docs or look for another solution before I settle for this. If we don't have another solution then we'll roll with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found a better way to get the package version and to be sure it is the actual version installed by the user. However I don't think it should be a part of this PR. I'll make the development in another PR. I'll just change your PR destination to develop
and in the meantime can you delete the modifications you did to the getReactScriptsVersion
function, please?
* react-scripts > v2.1.1 don't have a webpack.config.dev * make isEjected work of config/paths.js
webpack.config exposes config as a function which needs to be called with an env name.