Skip to content

Fixes #1684 when installing with yarn online #1685

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fixes #1684 when installing with yarn online #1685

wants to merge 1 commit into from

Conversation

lpalmes
Copy link
Contributor

@lpalmes lpalmes commented Feb 28, 2017

This was a quick fix of #1684 which i used to create a new project and the "false" : "0.0.4" dependency was gone.

lpalmes
lpalmes commented Feb 28, 2017
View reviewed changes
packages/create-react-app/index.js
@@ -163,9 +163,13 @@ function install(useYarn, dependencies, verbose, isOnline) {
command = 'yarnpkg';
args = [
'add',
'--exact',
isOnline === false && '--offline'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried using a ternary operation here but they all failed, and i didn't want to apply a filter function after concat because i think it obscures the intent.

@lpalmes lpalmes mentioned this pull request Feb 28, 2017
Closed
@Timer
Copy link
Contributor

Timer commented Feb 28, 2017

Oops! I resolved this in master already.
Sorry about this, I didn't see this PR and I started working on the bug immediately when I saw the issue. 😨

I really hope to see contributions from you in the future.

@Timer
Copy link
Contributor

Timer commented Feb 28, 2017

This is out in [email protected]. Please verify it works. 😄

@Timer Timer closed this Feb 28, 2017
@lpalmes
Copy link
Contributor Author

lpalmes commented Feb 28, 2017

That's fine, just wanted to fix this for the sake of the project, i hope i will have some contributions in the future 😄 .
Thanks tim, i'm going to close this then, i guess you can close the issue now that has been fixed 🎉

@lpalmes
Copy link
Contributor Author

lpalmes commented Feb 28, 2017

@Timer
Ahhh, much better. Glad this didn't evolve into leftpad 😆

"dependencies": {
    "react": "^15.4.2",
    "react-dom": "^15.4.2"
  },

@Timer
Copy link
Contributor

Timer commented Feb 28, 2017

Thanks for catching it! If you'd like, I'd appreciate an added test case which makes sure we don't install any dependencies other than those 3.

@lpalmes
Copy link
Contributor Author

lpalmes commented Feb 28, 2017

Sure, i can take a shot at it, should that be in the file, like check for the correct dependencies and if we found other than react, react-dom, react-scripts just process.exit(1)?

@Timer
Copy link
Contributor

Timer commented Mar 1, 2017

Yeah you can add it to e2e-installs.sh and either call a node script that inspects the package or use a few unix commands. Unix commands would probably be preferred, but Node totally acceptable.

@Timer
Copy link
Contributor

Timer commented Mar 1, 2017 •
edited
Loading

I'm sure you could use awk and accomplish this, or grep depending on the version.

@gaearon
Copy link
Contributor

gaearon commented Mar 1, 2017

Thanks for the quick fix. 😅

@gaearon gaearon mentioned this pull request Mar 5, 2017
Closed
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lpalmes @Timer @gaearon @facebook-github-bot