Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@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

Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@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

Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@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

Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@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

Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@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

Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@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

Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@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

Changes error code to when test is undefined - #91

Closed
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1
Closed

Changes error code to when test is undefined#91
deiga wants to merge 4 commits into
npm:latestfrom
deiga:changes-npm-test-exit-code-to-1

Conversation

@deiga

@deigadeiga commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga
deiga requested a review from a team as a code ownerNovember 4, 2018 20:39
@zkatzkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat

zkat commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga

Copy link
Copy Markdown
ContributorAuthor

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

Comment threadtest/tap/run-script.js Outdated
@isaacs

Copy link
Copy Markdown
Contributor

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs

Copy link
Copy Markdown
Contributor

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga

Copy link
Copy Markdown
ContributorAuthor

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs

Copy link
Copy Markdown
Contributor

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarkedarcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deigaClose: #91
Reviewed-by: @isaacs
NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke

Copy link
Copy Markdown
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

github-actionsBot added a commit to Kevinlee7250/cli that referenced this pull request Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.xwork is associated with a specific npm 7 releasesemver:majorbackwards-incompatible breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deiga@zkat@isaacs@darcyclarke