repl: avoid interpreting 'npm' as a command when errors are recoverable #54848
+24
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
needs-ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54848 +/- ##
==========================================
+ Coverage 87.62% 88.06% +0.43%
==========================================
Files 650 651 +1
Lines 182983 183388 +405
Branches 35406 35800 +394
==========================================
+ Hits 160336 161496 +1160
+ Misses 15917 15160 -757
- Partials 6730 6732 +2
|
RedYetiDev
reviewed
Sep 8, 2024
test/parallel/test-repl.js
Outdated
| }, | ||
| ...possibleTokensAfterIdentifier.map((token) => ( | ||
| { | ||
| send: `npm ${token}; undefined`, |
There was a problem hiding this comment.
Nice idea, that's definitely the right path... however the test right here isn't an accurate measurement of whether this works. The tests for NPM need to be a recoverable error, however these lines are valid JS.
Try adding a newline character in the middle of the recoverable statement to see if it works.
There was a problem hiding this comment.
AFAICT this test already passes on main. A test that validates this change should fail on main and pass on the PR branch.
There was a problem hiding this comment.
That's the issue with the test. The test needs to throw a recoverable syntax error, however these are valid JS.
There was a problem hiding this comment.
You're right, with this, it doesn't go through the recoverable error path.
I have added new tests with line breaks included.
This change ensures that 'npm' within JavaScript code is not mistakenly interpreted as an npm command when the error is recoverable. This allows 'npm' to be treated as expected in such scenarios. Fixes: nodejs#54830
islandryu
force-pushed
the
fix/repl-npm
branch
from
c3f8b21
to
8892202
Compare
RedYetiDev
reviewed
Sep 9, 2024
test/parallel/test-repl.js
Outdated
Comment on lines
412
to
422
| { | ||
| send: 'let npm = () => {};', | ||
| expect: 'undefined' | ||
| }, | ||
| ...possibleTokensAfterIdentifier.map((token) => ( | ||
| { | ||
| send: `npm ${token}; undefined`, | ||
| expect: 'undefined' | ||
| } | ||
| )), | ||
| { |
There was a problem hiding this comment.
I dont think this test is needed.
There was a problem hiding this comment.
I have removed it.
|
CC @nodejs/repl |
aduh95
approved these changes
Sep 11, 2024
aduh95
added
author ready
|
Can you fix the linter error please? |
aduh95
removed
the
author ready
RedYetiDev
added
the
commit-queue-squash
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
jasnell
approved these changes
Sep 13, 2024
cola119
approved these changes
Sep 15, 2024
cola119
added
author ready
nodejs-github-bot
removed
the
commit-queue
|
Landed in 86bdca9 |
RafaelGSS
pushed a commit
that referenced
this pull request
Sep 16, 2024
This change ensures that 'npm' within JavaScript code is not mistakenly interpreted as an npm command when the error is recoverable. This allows 'npm' to be treated as expected in such scenarios. Fixes: #54830 PR-URL: #54848 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Merged
RafaelGSS
pushed a commit
that referenced
this pull request
Sep 16, 2024
This change ensures that 'npm' within JavaScript code is not mistakenly interpreted as an npm command when the error is recoverable. This allows 'npm' to be treated as expected in such scenarios. Fixes: #54830 PR-URL: #54848 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
RafaelGSS
pushed a commit
that referenced
this pull request
Sep 17, 2024
This change ensures that 'npm' within JavaScript code is not mistakenly interpreted as an npm command when the error is recoverable. This allows 'npm' to be treated as expected in such scenarios. Fixes: #54830 PR-URL: #54848 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
RafaelGSS
pushed a commit
that referenced
this pull request
Sep 17, 2024
This change ensures that 'npm' within JavaScript code is not mistakenly interpreted as an npm command when the error is recoverable. This allows 'npm' to be treated as expected in such scenarios. Fixes: #54830 PR-URL: #54848 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change ensures that 'npm' within JavaScript code is not mistakenly interpreted as an npm command when the error is recoverable. This allows 'npm' to be treated as expected in such scenarios.
Fixes: #54830