Fix using pre-release tags with a tarball url in `--scripts-version` #876

jihchi · 2016-10-09T02:05:47Z

For example, react-scripts version0.2.0-alpha.1 with tarball url didn't works:

$ create-react-app --scripts-version=https://registry.npmjs.org/react-scripts/-/react-scripts-0.2.0-alpha.1.tgz test-app-tarball-url
Creating a new React app in /Users/archie/Repos/test-app-tarball-url.

Installing packages. This might take a couple minutes.
Installing react-scripts from npm...


> fsevents@1.0.14 install /Users/archie/Repos/test-app-tarball-url/node_modules/fsevents
> node-pre-gyp install --fallback-to-build

...

module.js:457
    throw err;
    ^

Error: Cannot find module '/Users/archie/Repos/test-app-tarball-url/node_modules/react-scripts-0.2.0/package.json'
    at Function.Module._resolveFilename (module.js:455:15)
    at Function.Module._load (module.js:403:25)
    at Module.require (module.js:483:17)
    at require (internal/module.js:20:19)
    at checkNodeVersion (/Users/archie/.nvm/versions/node/v6.7.0/lib/node_modules/create-react-app/index.js:170:21)
    at ChildProcess.<anonymous> (/Users/archie/.nvm/versions/node/v6.7.0/lib/node_modules/create-react-app/index.js:127:5)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at maybeClose (internal/child_process.js:877:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:226:5)

create-react-app version: 0.5.0

gaearon · 2016-10-11T11:40:12Z

Thanks for the PR! Can you please explain why this works and the other one didn't?
Also it would need rebasing.

jihchi · 2016-10-11T14:18:19Z

create-react-app is expected that getPackageName returns exactly react-scripts string while processing tarball URL, for example:

getPackageName('https://registry.npmjs.org/react-scripts/-/react-scripts-0.2.0-alpha.1.tgz');
// expected: react-scripts
// actual: react-scripts-0.2.0

Current regex didn't handle pre-release tags.

This modification adds support for this case as well as original case.

Just like original regex, first it searches first backslash /^.+\/, then capture the group name (.+), and then the version -[0-9]+\.[0-9]+\.[0-9]+ (major-minor-patch), then a non-capture group (?:[-+].+)? which is optional and searches for pre-release tags, the last is tarball extension \.tgz$.

gaearon · 2016-10-11T14:25:27Z

packages/create-react-app/index.js

@@ -153,7 +153,7 @@ function getInstallPackage(version) {
 // Extract package name from tarball url or path.
 function getPackageName(installPackage) {
  if (installPackage.indexOf('.tgz') > -1) {
-    return installPackage.match(/^.+\/(.+)-.+\.tgz$/)[1];
+    return installPackage.match(/^.+\/(.+)-[0-9]+\.[0-9]+\.[0-9]+(?:[-+].+)?\.tgz$/)[1];


Let's add a comment showing intended inputs and what should match them

Comment added.

gaearon · 2016-10-11T15:41:58Z

packages/create-react-app/index.js

@@ -153,6 +153,8 @@ function getInstallPackage(version) {
 // Extract package name from tarball url or path.
 function getPackageName(installPackage) {
  if (installPackage.indexOf('.tgz') > -1) {
+    // The package name could be with or without semver version, e.g. react-scripts-0.2.0-alpha.1.tgz
+    // However, This function returns package name only wihout semver version.
    return installPackage.match(/^.+\/(.+)-[0-9]+\.[0-9]+\.[0-9]+(?:[-+].+)?\.tgz$/)[1];


Can we make regex cut everything after the first -\d?

Code changed.

gaearon · 2016-10-11T15:53:21Z

packages/create-react-app/index.js

-    return installPackage.match(/^.+\/(.+)-.+\.tgz$/)[1];
+    // The package name could be with or without semver version, e.g. react-scripts-0.2.0-alpha.1.tgz
+    // However, This function returns package name only wihout semver version.
+    return installPackage.match(/^.+\/(.+)-\d+.+\.tgz$/)[1];


Can we make that group optional? So that just react-scripts.tgz will also get parsed correctly.

Code changed.

Regex breakdown:

^.+\/ starts with any characters until last slash

( group 1

.+? searches for package name (non-greedy for searching optional semver version)

)

(?: non-capturing group

-\d+ searches for version starts with dash & number

.+ cut everything after

)? it's optional

\.tgz$ finally, the tarball file extension

gaearon · 2016-10-11T15:53:29Z

packages/create-react-app/index.js

@@ -153,7 +153,9 @@ function getInstallPackage(version) {
 // Extract package name from tarball url or path.
 function getPackageName(installPackage) {
  if (installPackage.indexOf('.tgz') > -1) {
-    return installPackage.match(/^.+\/(.+)-.+\.tgz$/)[1];
+    // The package name could be with or without semver version, e.g. react-scripts-0.2.0-alpha.1.tgz
+    // However, This function returns package name only wihout semver version.


Nit: "This" -> "this".

gaearon · 2016-10-12T12:46:13Z

Looks good to me. Thanks!

…acebook#876) * Add supports for prelease tags version * Add comment to regex * Cut everything after the first -\d * Make semver version optional, so just package name get parsed correctly

facebook-github-bot added the CLA Signed label Oct 9, 2016

Add supports for prelease tags version

4cadd4f

facebook-github-bot added the CLA Signed label Oct 11, 2016

gaearon suggested changes Oct 11, 2016

View reviewed changes

Add comment to regex

9234a7d

gaearon reviewed Oct 11, 2016

View reviewed changes

Cut everything after the first -\d

a27ca1d

gaearon suggested changes Oct 11, 2016

View reviewed changes

Make semver version optional, so just package name get parsed correctly

f47475f

gaearon added this to the 0.7.0 milestone Oct 12, 2016

gaearon approved these changes Oct 12, 2016

View reviewed changes

gaearon merged commit c5f5b00 into facebook:master Oct 12, 2016

lock bot locked and limited conversation to collaborators Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix using pre-release tags with a tarball url in `--scripts-version` #876

Fix using pre-release tags with a tarball url in `--scripts-version` #876

jihchi commented Oct 9, 2016

gaearon commented Oct 11, 2016

jihchi commented Oct 11, 2016 •

edited

Loading

gaearon Oct 11, 2016

jihchi Oct 11, 2016

gaearon Oct 11, 2016

jihchi Oct 11, 2016 •

edited

Loading

gaearon Oct 11, 2016

jihchi Oct 12, 2016 •

edited

Loading

gaearon Oct 11, 2016

jihchi Oct 12, 2016

gaearon commented Oct 12, 2016

Fix using pre-release tags with a tarball url in --scripts-version #876

Fix using pre-release tags with a tarball url in --scripts-version #876

Conversation

jihchi commented Oct 9, 2016

gaearon commented Oct 11, 2016

jihchi commented Oct 11, 2016 • edited Loading

gaearon Oct 11, 2016

Choose a reason for hiding this comment

jihchi Oct 11, 2016

Choose a reason for hiding this comment

gaearon Oct 11, 2016

Choose a reason for hiding this comment

jihchi Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

gaearon Oct 11, 2016

Choose a reason for hiding this comment

jihchi Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

gaearon Oct 11, 2016

Choose a reason for hiding this comment

jihchi Oct 12, 2016

Choose a reason for hiding this comment

gaearon commented Oct 12, 2016

Fix using pre-release tags with a tarball url in `--scripts-version` #876

Fix using pre-release tags with a tarball url in `--scripts-version` #876

jihchi commented Oct 11, 2016 •

edited

Loading

jihchi Oct 11, 2016 •

edited

Loading

jihchi Oct 12, 2016 •

edited

Loading