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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cli] Replace
update-notifier
dependency with built in #8090[cli] Replace
update-notifier
dependency with built in #8090Changes from 13 commits
c38e118
f8b06e0
01fd56d
d36c1ec
03d0fdb
81be569
c46800d
c87c22a
0d72cb2
21494cb
d602459
9d5660c
6a48252
11c4129
283d91d
9981b47
cbba870
eb3713e
c10679d
7edf3b4
b8cbb20
469cf96
0597798
e952a1f
5b7fce5
21957af
055b908
5a021cb
a652572
250c83d
87c6399
1225467
035aea4
9ef6d5d
c2ad59b
78a79e1
72835e8
118cd7b
d7a122c
2735cb5
3bb5005
1633be7
d1c15d5
e46eadb
da2d962
7dbeda7
21b5106
fb9ae3c
14edff0
4cd39b0
abc40e3
8caddf5
d23ef97
194d888
cd50dfa
a30df0e
975d0a7
35f0f8c
53490b7
8f8508d
d41e739
4287e2d
99c82c0
286dd6a
6d45a3f
c91e5da
1a768cc
2a3540f
de4b687
205b6c8
a3cb3b0
c08c50c
7d040cb
def2926
226a232
5523b07
c765778
e1297f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking - what if dependencies is empty? Should we still log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update this in a follow-up PR?
We probably don't want to log if
dependencies
is empty. Or, at least we'd want to log a clearer message saying so.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On one hand, this is a build script and if there are no dependencies, then I'd think we'd want to see that.
On the other hand,
dependencies
will likely never be empty.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using
output.log()
here instead ofconsole.log()
otherwise we'll break the pipe-ability of commands (i.e. deployment URL goes to stdout forvc deploy
).I realize that this was the previous behavior as well, but since we're here we might as well fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the
isTTY
check, this won't break piped output, but by convention and for future testing, we should still probably change these to useoutput
in a follow-up PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update this in a follow-up PR?
This was about using
output
instead ofconsole.log
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try
undici
here? As a company we are trying to start moving away from node-fetch and using the fetch included with Node.js (which is undici)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with switching to undici eventually, but that's out-of-scope for this PR.