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

Install the self-contained binary version of pnpm #31

Closed
wants to merge 3 commits into from
Closed

Conversation

zkochan
Copy link
Member

@zkochan zkochan commented Feb 9, 2022

This is a breaking change because the binary version of pnpm is only available from v6.17.1.
Also, it doesn't ship pnpx.

close #18

This is a breaking change because the binary version of pnpm
is only available from v6.17.1

close #18
Copy link
Collaborator

@KSXGitHub KSXGitHub left a comment

Choose a reason for hiding this comment

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

  1. You intend to drop support for pnpm versions without self-contained binary, correct? If that's the case, why even bother running the self-installer and install it via the npm registry at all? Why not just download the binary directly from GitHub release?
  2. Can you add code that check for unsupported pnpm versions before download? If user specify a pnpm version that doesn't have self-contained binary, it is more user-friendly to inform the user about it than an ambiguous 404.
  3. Can you update the README.md file? Some information in README.md is ought to be outdated after this PR.
  4. Finally, please remember to update dist/index.js before commit.

@KSXGitHub
Copy link
Collaborator

Amendment:

You intend to drop support for pnpm versions without self-contained binary, correct? If that's the case, why even bother running the self-installer and install it via the npm registry at all? Why not just download the binary directly from GitHub release?

GitHub Release does not support version ranges (e.g. ^6.17.1), so we must resolve to the correct version ourselves. Or just fallback to npm. Or drop support for version ranges entirely.

@KSXGitHub
Copy link
Collaborator

@zkochan When will you resolve my requests for change in this PR?

@zkochan
Copy link
Member Author

zkochan commented Feb 21, 2022

I have to think about it. Not soon.

@privatenumber
Copy link
Sponsor

Curious if this would be easier to move forward with if it's opt-in via an option (eg. binary: true)?

@tksst tksst mentioned this pull request Mar 29, 2023
@tksst
Copy link
Contributor

tksst commented Mar 30, 2023

I agree with @privatenumber .
Since this branch is very old and differs significantly from the latest master, I forked this action and implemented an option to install a self-contained binary.

I will post another PR after I add the test and update the readme.

@fz6m
Copy link

fz6m commented Apr 1, 2023

Good job @tksst .

I think adding a node-version option, detect it and auto switch pnpm@version to @pnpm/exe@version is better.

e.g.

jobs:
  build:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        node-version: [14.x, 16.x, 18.x]
        os: [ubuntu-latest, windows-latest]
    steps:
      - name: Install pnpm
        uses: pnpm/action-setup@v3.0.0
        with:
          # in the node 14.x env, we auto install `@pnpm/exe` instead of `pnpm`
          # refer: https://pnpm.io/installation#compatibility
          node-version: ${{ matrix.node-version }}
          version: 8

      - name: Use Node.js ${{ matrix.node-version }}
        uses: actions/setup-node@v3
        with:
          node-version: ${{ matrix.node-version }}
          cache: 'pnpm'

@tksst
Copy link
Contributor

tksst commented Apr 2, 2023

@fz6m

I think adding a node-version option, detect it and auto switch pnpm@version to @pnpm/exe@version is better.

That is a nice feature, but a bit complicated.
I may try to implement it when I have time.

@tksst
Copy link
Contributor

tksst commented Apr 4, 2023

After thinking about how to implement this feature, I gave up trying to implement it.
The node-version option of setup-node can take various values. For example, "^18.0.0", ">=18.0.0", "lts/*", etc.
I need to implement the same logic to determine the version as actions/setup-node. This is difficult.

@KSXGitHub KSXGitHub mentioned this pull request Jul 26, 2023
@zkochan zkochan closed this in #92 Jul 26, 2023
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.

Feature request: run pnpm with old version node
5 participants