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 all 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.
I think this needs to clean up the lock file. Otherwise the user will never be able to update again.
I'm wondering if we even need a lock file at all 🤔
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.
This
setTimeout()
exists in the event the parent never sends the message, thus no lock file will be created to be cleaned up. The lock file name is derived from the cache file name, which is the package name plus the dist tag. Those two bits of info are not available at the time the timer is created.We could rename
timer
towaitForParentTimer
, then clear it once it gets the message to start, then create anAbortController
to make sure thefetch()
returns in a reasonable time.Lock files are only 4KB on disk and I have yet to come across a scenario where the lock file wasn't cleaned up. I'm sure it will happen at some point and that's why I added the stale pid check in
isRunning()
.As for needing the lock file, I did some testing and it's possible to run
vercel
commands fast enough that one worker is clobbering the cache file with stale data (e.g. thenotified
flag being toggled). Since the access to the cache file should be atomic, a lock file will synchronize multiple processes ensuring a predictable state. Obviously we're not building a database, so if you feel the lock file, or the whole IPC design for that matter, is overkill, I'd be happy to refactor the code.