Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fixed "publish --force" not correctly publishing for already published packages - #565

Closed
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1
Closed

Fixed "publish --force" not correctly publishing for already published packages#565
MasterCassim wants to merge 1 commit into
npm:latestfrom
MasterCassim:patch-1

Conversation

@MasterCassim

@MasterCassimMasterCassim commented Dec 6, 2019

Copy link
Copy Markdown

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), :14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}

The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.

What / Why

  • It is no longer possible to publish already published packages with the --force option.

References

Currently "publish --force" does not work correctly when the deployed package is already in the registry. This is a regression from v10 where this worked correctly. The following error is thrown in line 138 of publish.js:
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received <Buffer 1f 8b 08 00 4d 45 ea 5d 04 00 ed 19 f9 af d2 30 d8 9f 4d fc 1f 2a 1a 07 ca 36 f0 8a 22 a0 c6 db 78 45 3c e3 95 b9 75 50 ...
at readFile (fs.js:295:10)
at go$readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:110:14)
at readFile (C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:107:12)
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at ret (eval at makeNodePromisifiedEval (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:14:23)
at upload (C:\Program Files\nodejs\node_modules\npm\lib\publish.js:138:12)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:156:22
at C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:16:12
at tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Function.Promise.attempt.Promise.try (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\method.js:39:29)
at otplease (C:\Program Files\nodejs\node_modules\npm\lib\utils\otplease.js:14:16)
at C:\Program Files\nodejs\node_modules\npm\lib\publish.js:154:20
at PassThroughHandlerContext.finallyHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\finally.js:56:23)
at PassThroughHandlerContext.tryCatcher (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\util.js:16:23)
at Promise._settlePromiseFromHandler (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:517:31)
at Promise._settlePromise (C:\Program Files\nodejs\node_modules\npm\node_modules\bluebird\js\release\promise.js:574:18) {
code: 'ERR_INVALID_ARG_VALUE'
}
The BB.promisify(require('graceful-fs').readFile) throws an error when used with an input buffer which is used in line 156 of publish.js. Using the file path again (PR) fixes the problem and is the same behavior as in v10.
@MasterCassim
MasterCassim requested a review from a team as a code ownerDecember 6, 2019 12:57
@ljharb

Copy link
Copy Markdown
Contributor

I'm confused; published versions are immutable and can't be overwritten. When does this apply?

@MasterCassim

MasterCassim commented Dec 6, 2019 via email

Copy link
Copy Markdown
Author

@ljharb

ljharb commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Right, but even when unpublishing succeeds, because it's within the short window where that's possible, you can never republish the same version ever again even after it's unpublished.

@MasterCassim

MasterCassim commented Dec 6, 2019

Copy link
Copy Markdown
Author

But isn't that exactly what the code in publish,js does? Call libpub -> In case of EPUBLISHCONFLICT calls libunpub, then calls upload again which then calls libpub. However, the second libpub is never called because of the mentioned bug (which worked in v10) although the second upload is actually called.

function upload (pkg, isRetry, cached, opts) {
if (!opts.dryRun) {
return readFileAsync(cached).then(tarball => {
return otplease(opts, opts => {
return libpub(pkg, tarball, opts)
}).catch(err => {
if (
err.code === 'EPUBLISHCONFLICT' &&
opts.force &&
!isRetry
) {
log.warn('publish', 'Forced publish over ' + pkg._id)
return otplease(opts, opts => libunpub(
npa.resolve(pkg.name, pkg.version), opts
)).finally(() => {
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, cached, opts)
}).catch(() => {
// but if it fails again, then report the first error.
throw err
})
})
} else {
throw err
}
})
})
} else {
return opts.Promise.resolve(true)
}
}

Even if not supported by the default npm registry; we are using nexus repository which allows republishing of the same version. Although other local repositories (verdaccio) also allow republish of the same version.

@ljharb

Copy link
Copy Markdown
Contributor

Gotcha; that's probably why nobody noticed, since i think most people just take it for granted that published things are immutable now (there's lots of security and maintenance reasons to have them so). Sorry for the confusion.

@MasterCassim

Copy link
Copy Markdown
Author

No problem. We have a pretty complicated setup which just recently included npm modules as well. Because we are coming from maven (java) world where our version number only changes for external releases; we implemented the same for our new npm modules.

There were some hurdles but in the end, this worked correctly with nodejs 10 but broke once we upgraded to nodejs 12.

Comment threadlib/publish.js
// ignore errors. Use the force. Reach out with your feelings.
return otplease(opts, opts => {
return upload(pkg, true, tarball, opts)
return upload(pkg, true, cached, opts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be replacing upload with libpub, not replacing tarball with cached. Note that the function it's retrying is libpub, not upload?

We need a test in any event, and probably some more investigation.

@ruyadornoruyadorno added Needs Discussion is pending a discussion pr: needs tests requires tests before merging labels Jan 6, 2020
@darcyclarkedarcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@darcyclarkedarcyclarke added Bug thing that needs fixing semver:patch semver patch level for changes and removed Needs Discussion is pending a discussion Bug thing that needs fixing labels Oct 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs testsrequires tests before mergingRelease 6.xwork is associated with a specific npm 6 releasesemver:patchsemver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MasterCassim@ljharb@isaacs@ruyadorno@darcyclarke