-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix(open): set wait: false
to run server.close successfully
#2001
Conversation
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.
Good catch 👍
Codecov Report
@@ Coverage Diff @@
## master #2001 +/- ##
=======================================
Coverage 92.77% 92.77%
=======================================
Files 29 29
Lines 1149 1149
Branches 327 327
=======================================
Hits 1066 1066
Misses 79 79
Partials 4 4
Continue to review full report at Codecov.
|
@evilebottnawi I added one commit(fdc6d31). PTAL |
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.
No need modify object, it is really bad practice
lib/utils/runOpen.js
Outdated
openOptions = { app: options.open }; | ||
Object.assign(openOptions, { | ||
app: options.open, | ||
}); |
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.
let openOptions = { wait: false };
// ...
if (typeof options.open === 'string') {
openOptions = Object.assign({} openOptions, { app: options.open });
}
No need modify openOptions
variable, it is very bad practice
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.
openOptions = Object.assign({} openOptions, { app: options.open });
equals Object.assign(openOptions, { app: options.open })
.
I don't think this is a very bad practice.
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.
Yes, but it is not good, we modify original object and it is bad practice because you can forget about this in future and it is lead to some problems when you refactor code.
When you pass options to third party package(s) better always cloning object. Other package can modify object too so it can lead to very strange errors and situations.
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.
When you pass options to third party package(s) better always cloning object. Other package can modify object too so it can lead to very strange errors and situations.
If you say, we should not assign to the same variable.
I don't have any strong opinions, so I'll change.
a64edce
to
abeaa9c
Compare
@evilebottnawi PTAL |
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.
Good work!
fixes: #1990
For Bugs and Features; did you add new tests?
yes
Motivation / Use-Case
see #1990
commit: sindresorhus/open@da2d663#diff-168726dbe96b3ce427e7fedce31bb0bcR23
Our open(opn)'s version is old, and
wait
seems to betrue
by default in this version.Breaking Changes
no
Additional Info