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

Support next as npm tag in --scripts-version #4348

Closed
miraage opened this issue Apr 23, 2018 · 7 comments
Closed

Support next as npm tag in --scripts-version #4348

miraage opened this issue Apr 23, 2018 · 7 comments

Comments

@miraage
Copy link

miraage commented Apr 23, 2018

Not sure if it is a bug or not (might be intented behavior).
Instead of writing ... --scripts-version=2.0.0-next.66cc7a90 we would write ... -scripts-version=next.

Currently it installs zeit/next.js.

// EDIT

Possible solution might touch a single line:

- if (validSemver) {
+ if (validSemver || version === 'next') {

Not sure how "breaking" this change might be.

@gaearon
Copy link
Contributor

gaearon commented Apr 23, 2018

Currently it installs zeit/next.js.

😄

Possible solution might touch a single line:

It's odd to treat next specifically in a different way. Maybe we can support @next (i.e. treat @ as beginning of npm tag)?

@miraage
Copy link
Author

miraage commented Apr 23, 2018

Sounds good to me. Kinda explicit package version.

@Timer
Copy link
Contributor

Timer commented Apr 23, 2018

Do you want to send a PR?

@miraage
Copy link
Author

miraage commented Apr 23, 2018

@Timer I can do it. I have a question regarding resolving package version. I see a few options:

  1. simply check if the first character is @ and string length is greater than 1 (semver.valid returns false if we pass next).
  2. check for a valid versions using npm view react-scripts@next version. It has 3 kind of responses (ok, package 404, bad tag). There is also some options:
  • 2.1. fallback to the latest stable tag (silently or with a warning?)
  • 2.2. throw an error

/cc @gaearon

@miraage
Copy link
Author

miraage commented Apr 23, 2018

There is an issue with yarn: version outputs as is.

$ yarn info react-scripts@next version --silent
> next

https://github.com/yarnpkg/yarn/blob/v1.6.0/src/cli/commands/info.js#L77

We might want

- result.version = version || result['dist-tags'].latest;
+ result.version = result['dist-tags'][version] || version || result['dist-tags'].latest;

Should I open a PR there?

@Timer
Copy link
Contributor

Timer commented Apr 24, 2018

I'm guessing just a simple check; if the string starts with @ just do .substring(1) and treat that as the version (the same way 2.0.0-next.66cc7a90 is).

@miraage
Copy link
Author

miraage commented Apr 24, 2018

It makes sense since other options are not validated as well (e.g. file: or .tar.gz). WIP.

Timer pushed a commit that referenced this issue Apr 27, 2018
* Support package distribution tags (#4348)

* Remove redundand variable check in `getInstallPackage`

* Simplify react-scripts version using `--scripts-version=@tagname` notation

* Add dist-tag tests to e2e-installs
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants