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

Improve package upgrade prompt #374

Closed
wants to merge 1 commit into from
Closed

Improve package upgrade prompt #374

wants to merge 1 commit into from

Conversation

bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Apr 11, 2024

TBH I'm not sure if even this is worth doing. I think everyone who uses a package manager other than NPM knows how to convert the standard "npm install" message to their own package-manager-of-choice.

@bvaughn bvaughn requested a review from Andarist April 11, 2024 21:10
Copy link

replay-io bot commented Apr 11, 2024

Status Complete ↗︎
Commit 5cc7204
Results
2 Failed
  • clicks a disappearing button
  • should fail on this test
  • 42 Passed
  • adds items
  • adds new items using a custom command
  • adds todos following the fixture
  • adds todos following the fixture
  • adds todos following the fixture
  • adds todos following the fixture
  • calls inform
  • complete all checkbox should update state when items are completed / cleared
  • gets a number
  • only gets a number
  • only gets a number
  • should allow me to add todo items
  • should allow me to clear the complete state of all items
  • should allow me to display active items
  • should allow me to display all items
  • should allow me to display completed items
  • should allow me to edit an item
  • should allow me to mark all items as completed
  • should allow me to mark items as complete
  • should allow me to un-mark items as complete
  • should append new items to the bottom of the list
  • should be hidden when there are no items that are completed
  • should cancel edits on escape
  • should clear text input field when an item is added
  • should display the correct text
  • should display the current number of todo items
  • should focus on the todo input field
  • should hide #main and #footer
  • should hide other controls when editing
  • should highlight the currently applied filter
  • should intercept postman
  • should invoke some commands that have exceptional option handling
  • should log
  • should persist its data
  • should remove completed items when clicked
  • should remove the item if an empty text string was entered
  • should respect the back button
  • should save edits on blur
  • should show #main and #footer when items added
  • should trim entered text
  • should trim text input
  • yields a number
  • return "yarn";
    }

    // Otherwise let's see which package manager(s) are installed
    Copy link
    Member

    Choose a reason for hiding this comment

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

    I have all of them installed. It would be better to lookup the closest lockfile

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    Hm. I dunno. I think the likelihood of you running the CLI from within a project folder that also happens to be the way you installed replayio seems low. I either run it from my home folder (default terminal location) or I run it from whatever project I'm working in (which may use Yarn or PNPM, but that's not how I globally installed the CLI)


    const execAsync = promisify(exec);

    export async function getLatestPackageVersion(command: PackageManager, packageName: string) {
    Copy link
    Member

    Choose a reason for hiding this comment

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

    I probably wouldn't even mind just using npm for this. Your version is better/more robust but even if a project uses an alternative package manager they will likely still have npm available.

    But we could even go a step further - spawning an extra process for this is already a little bit wasteful. So we could optimize this by making a direct API call:

      const response = await fetch("https://registry.npmjs.org/replayio", {
        headers: {
          // request abbreviated metadata
          Accept: "application/vnd.npm.install-v1+json",
        },
      });
      const json = await response.json();
      const latestVersion = json["dist-tags"]?.latest;

    This is documented here

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    That seems like an improvement.

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    console.log("Press any key to continue");
    console.log("");
    const command = getPackageManagerCommand();
    if (command) {
    Copy link
    Member

    Choose a reason for hiding this comment

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

    I think everyone who uses a package manager other than NPM knows how to convert the standard "npm install" message to their own package-manager-of-choice.

    Right. So I wasn't worried about what we print here - just that we don't execute the command when the user confirms the prompt. We execute runtime installs - that's of course much more helpful since the user doesn't have to know how to do that from the top of their head. It's just that when we are prompting for both it feels off that they both don't execute things.

    That was my literal confusion as the user here. I was first prompted about the runtime update and later about the npm update (during some subsequent run). At that point, I was just expecting that it would also perform the installation for me but it didn't.

    Copy link
    Member

    Choose a reason for hiding this comment

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

    I think that without this auto-installation it might not be worth pulling in this PR. I understand that auto-installation isn't exactly a no-brainer with a plethora of alternative package managers on the market, use cases and setups. So staying away from it might very well be just the right thing to do to avoid edge cases and weird problems.

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    Having looked into this, and thought more about the edge cases, I'm inclined to agree. I posted this PR for discussion but I'm happy to bail on it and just do this one thing instead.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants