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

Add install command support for package version #2161

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

realies
Copy link
Contributor

@realies realies commented Mar 6, 2018

Perhaps also replace packageName to json.name (if available) after line 33.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Mar 6, 2018
@astorije
Copy link
Member

astorije commented Mar 7, 2018

Uh oh! @realies, do you need any help with this?

@realies realies reopened this Mar 7, 2018
@astorije
Copy link
Member

astorije commented Mar 7, 2018

Alright so after chatting on IRC, documenting here steps to clean this PR in case it helps.

You'll notice that your PR says: "This branch has conflicts that must be resolved - Conflicting files: src/command-line/install.js", and the changed files show changes that aren't yours. In fact, those changes are the reason of the conflicting files.

To fix this, you need to change your commits so it only contains your stuff. Note that all that follows has to happen in your terminal, not on the GitHub UI, so make sure you are in a terminal, you ran git pull on your own repo and your own branch. Then, to fix this:

  • The master branch in your fork be outdated from ours. Let's start by syncing those:
    • On your local repo, start by telling your git what is the point of reference, i.e. our upstream repo: git remote add upstream https://github.com/thelounge/thelounge
    • Make sure you don't have local changes (git status must be empty) and that you are in your master branch: git checkout master
    • Sync it with ours: git fetch upstream && git rebase upstream/master 
    • git status should tell you your local master is ahead of a few commits, run git push (don't --force it as it might be a sign that something is wrong) to update your fork.
  • Now, let's clean up your branch:
    • Go to your branch: git checkout patch-4
    • Sync it with master: git rebase master
    • 💥 Conflicts! (Probably)
    • In your situation, I would run git reset HEAD src/command-line/install.js, and clean up things manually.
    • When you're done cleaning things, tell git you're done fixing conflicts: git add src/command-line/install.js && git rebase --continue
    • Check that your commit is now looking good with git show
    • If everything looks alright, update your branch on GitHub, and therefore this PR: git push --force (this time you are going to need to --force it)

Let me know how this goes!

@realies
Copy link
Contributor Author

realies commented Mar 7, 2018

@astorije, the git magic guide is much appreciated. Saving for future reference.

packageJson(packageName, {
fullMetadata: true,
version: packageVersion,
Copy link
Member

Choose a reason for hiding this comment

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

Kinda sad we have to split this ourselves since most commands in the yarn/npm CLIs support passing version after @.

@realies, maybe open an issue on https://github.com/sindresorhus/package-json to see if they'd be open to also supporting packageJson("my-package@something")?
That way, all potential edge cases of version parsing would be handled there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @realies!

@@ -71,7 +76,7 @@ program
"--non-interactive",
"--cwd",
packagesPath,
`${packageName}@${json.version}`,
`${json.name}@${json.version}`,
Copy link
Member

Choose a reason for hiding this comment

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

I know this is not your doing but I'm just trying to clarify something with one of @thelounge/maintainers: is this way of passing the name (instead of just packageName) because we explicitly want to pass @latest? It doesn't work correctly without specifying dist-tag or version?

Copy link
Member

Choose a reason for hiding this comment

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

We used to support it. Then it was removed. Pretty sure I mentioned this at the time. Will check when letter.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so looking at this thread, it looks like I was confused by this change, but I think I understand it now 😅

Copy link
Member

Choose a reason for hiding this comment

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

It's used to specify the exact version that exists in the NPM registry. Before this PR it looked up latest via the API, and put that version in the command. This PR adds support for installing other versions or tags, as they are all looked up via the api.

@astorije astorije added this to the 3.0.0 milestone Mar 8, 2018
@xPaw xPaw merged commit 6fdd6d4 into thelounge:master Mar 8, 2018
@realies realies deleted the patch-4 branch March 15, 2018 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants