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

fix(publish): validate dist-tag #7459

Merged
merged 2 commits into from
May 1, 2024
Merged

fix(publish): validate dist-tag #7459

merged 2 commits into from
May 1, 2024

Conversation

reggi
Copy link
Contributor

@reggi reggi commented May 1, 2024

Currently as described in #7275 the publish command lacks the same check that npm install and npm dist-tag add have to validate that the tag is valid (url encoded correctly via npa.js here). There's many ways to do this, or places this can go, totally open to putting this in a more appropriate place if there is one.

I tested this locally:

Shown error locally
impose git:(main) ✗ lnpm publish --tag=@meow --dry-run
npm verbose cli /Users/reggi/.nvm/versions/node/v20.12.1/bin/node /Users/reggi/Documents/GitHub/cli/bin/npm-cli.js
npm info using npm@10.7.0
npm info using node@v20.12.1
npm silly config:load:file:/Users/reggi/Documents/GitHub/cli/npmrc
npm silly config:load:file:/Users/reggi/Documents/GitHub/impose/.npmrc
npm silly config:load:file:/Users/reggi/.npmrc
npm silly config:load:file:/Users/reggi/.nvm/versions/node/v20.12.1/etc/npmrc
npm verbose title npm publish
npm verbose argv "publish" "--tag" "@meow" "--dry-run"
npm verbose logfile logs-max:10 dir:/Users/reggi/.npm/_logs/2024-05-01T19_13_17_321Z-
npm verbose logfile /Users/reggi/.npm/_logs/2024-05-01T19_13_17_321Z-debug-0.log
npm verbose publish [ '.' ]
npm silly logfile start cleaning logs, removing 1 files
npm silly logfile done cleaning log files
npm notice
npm notice 📦  impose@2.1.2-test
npm notice Tarball Contents
npm notice 588B README.md
npm notice 2.6kB index.js
npm notice 601B package.json
npm notice Tarball Details
npm notice name: impose
npm notice version: 2.1.2-test
npm notice filename: impose-2.1.2-test.tgz
npm notice package size: 1.6 kB
npm notice unpacked size: 3.8 kB
npm notice shasum: f35c044ccc35dece301dd8c58811f67b380efe0c
npm notice integrity: sha512-rCNg1ITYFY5um[...]ujZctB8jCJkXg==
npm notice total files: 3
npm notice
npm verbose stack Error: Invalid tag name "@meow" of package "impose@@meow": Tags may not have any characters that encodeURIComponent encodes.
npm verbose stack     at invalidTagName (/Users/reggi/Documents/GitHub/cli/node_modules/npm-package-arg/lib/npa.js:118:15)
npm verbose stack     at fromRegistry (/Users/reggi/Documents/GitHub/cli/node_modules/npm-package-arg/lib/npa.js:406:13)
npm verbose stack     at resolve (/Users/reggi/Documents/GitHub/cli/node_modules/npm-package-arg/lib/npa.js:87:12)
npm verbose stack     at npa (/Users/reggi/Documents/GitHub/cli/node_modules/npm-package-arg/lib/npa.js:53:10)
npm verbose stack     at Publish.exec (/Users/reggi/Documents/GitHub/cli/lib/commands/publish.js:102:5)
npm verbose stack     at async module.exports (/Users/reggi/Documents/GitHub/cli/lib/cli/entry.js:74:5)
npm verbose cwd /Users/reggi/Documents/GitHub/impose
npm verbose Darwin 23.4.0
npm verbose node v20.12.1
npm verbose npm  v10.7.0
npm error code EINVALIDTAGNAME
npm error Invalid tag name "@meow" of package "impose@@meow": Tags may not have any characters that encodeURIComponent encodes.
npm verbose exit 1
npm verbose code 1

npm error A complete log of this run can be found in: /Users/reggi/.npm/_logs/2024-05-01T19_13_17_321Z-debug-0.log
➜  impose git:(main) ✗ npm dist-tag add test --dry-run

@reggi reggi requested a review from a team as a code owner May 1, 2024 19:19
@reggi reggi changed the title Validates the tag on publish fix: validates the tag on publish May 1, 2024
@reggi reggi changed the title fix: validates the tag on publish fix: validates tag on publish May 1, 2024
@wraithgar
Copy link
Member

Will definitely want a test for this one.

@wraithgar wraithgar changed the title fix: validates tag on publish fix: validate dist-tag on publish May 1, 2024
@wraithgar wraithgar changed the title fix: validate dist-tag on publish fix(publish): validate dist-tag May 1, 2024
@reggi
Copy link
Contributor Author

reggi commented May 1, 2024

Flakey Linux 20.x test?

    # Subtest: do no fancy handling for shellouts
        # Subtest: shellout with a numeric error code
            ok 1 - got expected exit code
            ok 2 - no noisy warnings
            ok 3 - no noisy warnings
            not ok 4 - should be equivalent strictly
              ---
              diff: |
                --- expected
                +++ actual
                @@ -1,2 +1,4 @@
                 Array [
                +  "Transformation error for",
                +  "Cannot read properties of undefined (reading 'node')",
                 ]
              at:
                line: 608
                column: 7
Error:                 file: test/lib/cli/exit-handler.js
                type: Test
              stack: |
                Test.<anonymous> (test/lib/cli/exit-handler.js:608:7)
              ...
            
            1..4
            # failed 1 of 4 tests
        not ok 1 - shellout with a numeric error code # time=1500.314ms
        
        # Subtest: shellout without a numeric error code (something in npm)
            ok 1 - got expected exit code
            ok 2 - bring the noise
            not ok 3 - should be equivalent strictly
              ---
              diff: |
                --- expected
                +++ actual
                @@ -1,3 +1,2 @@
                 Array [
                -  "",
                 ]
              at:
                line: 617
                column: 7
Error:                 file: test/lib/cli/exit-handler.js
                type: Test
              stack: |
                Test.<anonymous> (test/lib/cli/exit-handler.js:617:7)
              ...
            
            1..3
            # failed 1 of 3 tests
        not ok 2 - shellout without a numeric error code (something in npm) # time=85.711ms

@wraithgar
Copy link
Member

Flakey Linux 20.x test?

Yep, known issue in testing only. It's on the backlog.

@wraithgar wraithgar merged commit 4ab6cf4 into latest May 1, 2024
20 checks passed
@wraithgar wraithgar deleted the reggi/validate-pub-tag branch May 1, 2024 19:39
@github-actions github-actions bot mentioned this pull request May 1, 2024
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.

2 participants