Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

react-scripts > v2.1.1 don't have a webpack.config.dev #20

Merged
merged 6 commits into from
Jan 10, 2019

Conversation

looseend
Copy link
Contributor

@looseend looseend commented Jan 7, 2019

webpack.config exposes config as a function which needs to be called with an env name.

Nargonath and others added 2 commits November 18, 2018 16:15
webpack.config exposes config as a function which needs to be called with an env name.
Copy link
Owner

@Nargonath Nargonath left a 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)

scripts/index.js Show resolved Hide resolved
@Nargonath Nargonath self-assigned this Jan 7, 2019
@Nargonath Nargonath added the enhancement New feature or request label Jan 7, 2019
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)),
Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Owner

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?

utils/index.js Show resolved Hide resolved
@Nargonath Nargonath changed the base branch from master to develop January 10, 2019 14:36
utils/index.js Outdated Show resolved Hide resolved
utils/index.js Outdated Show resolved Hide resolved
@Nargonath Nargonath merged commit fb7bcea into Nargonath:develop Jan 10, 2019
Nargonath pushed a commit that referenced this pull request Jan 12, 2019
* react-scripts > v2.1.1 don't have a webpack.config.dev
* make isEjected work of config/paths.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants