Skip to content

Remove code duplication - #1241

Closed
EnoahNetzach wants to merge 1 commit into
react:masterfrom
EnoahNetzach:remove-code-cuplication
Closed

Remove code duplication#1241
EnoahNetzach wants to merge 1 commit into
react:masterfrom
EnoahNetzach:remove-code-cuplication

Conversation

@EnoahNetzach

@EnoahNetzachEnoahNetzach commented Dec 11, 2016

Copy link
Copy Markdown
Contributor

I just realized that with #1195 I duplicated some code. This PR just removes it.

EnoahNetzach
EnoahNetzach commented Dec 11, 2016
View reviewed changes
Comment threadpackages/create-react-app/index.js Outdated

@EnoahNetzachEnoahNetzachDec 11, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm not even sure if this line is still useful.
Looks like unreachable code.

@gaearongaearonDec 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This lets us increase minimal version with time if react-scripts imposes this but user doesn't update the CLI itself.

gaearon
gaearon reviewed Dec 11, 2016
View reviewed changes
Comment threadpackages/create-react-app/index.js Outdated

@gaearongaearonDec 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this check then?

@EnoahNetzachEnoahNetzachDec 12, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This allows us to check the minimal supported version asap for the whole cra to work.
The problem was that it took ~10 minutes with node v0.10 to get to an halt with error, using a lot of resources meanwhile as it still installs all the dependencies, even thou it warns about the problem.

@EnoahNetzach
EnoahNetzachforce-pushed the remove-code-cuplication branch from 125edbd to c140012CompareJanuary 26, 2017 17:26
@gaearon

gaearon commented Jan 9, 2018

Copy link
Copy Markdown
Contributor

This doesn't merge cleanly and I forgot what this is about. If it's still relevant please resubmit :-)

@gaearongaearon closed this Jan 9, 2018
@EnoahNetzach

EnoahNetzach commented Jan 9, 2018

Copy link
Copy Markdown
ContributorAuthor

I don't think this is still relevant 🤷‍♂️

@EnoahNetzach
EnoahNetzach deleted the remove-code-cuplication branch January 9, 2018 16:45
@locklockBot locked and limited conversation to collaborators Jan 20, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@gaearongaearongaearon left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signedtag: internal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@EnoahNetzach@gaearon@fson@facebook-github-bot