Skip to content
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.

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

Make --scripts-version work with forked react-scripts #632

Merged
merged 1 commit into from
Sep 18, 2016

Conversation

yesmeck
Copy link
Contributor

@yesmeck yesmeck commented Sep 12, 2016

Attempt to fix #631.

@gaearon gaearon added this to the 0.4.2 milestone Sep 12, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 12, 2016

Can you provide a test plan? Please see CONTRIBUTING.md.

@ghost ghost added the CLA Signed label Sep 12, 2016
@yesmeck
Copy link
Contributor Author

yesmeck commented Sep 12, 2016

Should I create a file like tasks/e2e.sh but for custom react-scripts testing purpose?

@yesmeck
Copy link
Contributor Author

yesmeck commented Sep 12, 2016

For testing, I have tried running following commands:

node global-cli/index.js --scripts-version=https://registry.npmjs.org/react-scripts/-/react-scripts-0.4.0.tgz myapp
node global-cli/index.js --scripts-version=$(pwd)/$(npm pack) myapp
node global-cli/index.js --scripts-version=0.4.0 myapp
node global-cli/index.js --scripts-version=feeble-scripts myapp

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

These two functions look confusing to me:

function getInstallPackage(version) {
  var packageToInstall = 'react-scripts';
  var validSemver = semver.valid(version);
  if (validSemver) {
    packageToInstall += '@' + validSemver;
  } else if (version) {
    // for tar.gz or alternative paths
    packageToInstall = version;
  }
  return packageToInstall;
}

function getInstallPackageName(version) {
  var validSemver = semver.valid(version);
  if (validSemver) {
    return 'react-scripts';
  } else if (/^(https?:\/\/|\/)/.test(version)) {
    return version.match(/^.+\/(.+)-.+\.tgz$/)[1];
  } else {
    return version;
  }
}

I don’t quite understand what they’re doing, how their return values are different, why they both hardcode react-scripts. It feels to me that they should be merged into a single function.

Can you please refactor so that they look less confusing, or best, merged?

@yesmeck
Copy link
Contributor Author

yesmeck commented Sep 18, 2016

These two functions exist for different purposes. Because of a package can be url or a path, so we need a function to get the package name from the url or path. I rewrite the getInstallPackageName function and add a little comment. Hope it's not that confusing now.

@gaearon gaearon merged this pull request into facebook:master Sep 18, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

Thanks!

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

This should be fixed in 0.4.2.
Please verify!

@yesmeck
Copy link
Contributor Author

yesmeck commented Sep 18, 2016

It works.

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

If you have the time I would appreciate if you could look into adding something to our e2e test so we don't regress.

@yesmeck
Copy link
Contributor Author

yesmeck commented Sep 18, 2016

Current e2e test only tests the normal workflow. But if we want test CRA with forked react-scripts, let's say create-react-app --scripts-version foobar-scripts myapp, it's hard to reuse current e2e test, because foobar-scripts may emit a different folder structure which fail the current asserts.

Should we create multiple e2e test case for different scenarios?

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

We don’t have to do the same asserts after this special run with a fork.

feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--scripts-version not working with forked react-scripts
3 participants