-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
prevent UnhandledPromiseRejectionWarning in node 6 #7655
Conversation
|
For maintainers only:
|
Ugghh. Since the code change was so minimal, I just copy-n-pasted from my local node_modules into the github web editor. I knew I should've taken the extra time to clone the repo and run tests! Anyways, I just fixed the style errors via |
When running tests locally, I see 2 failed tests:
But those also happen in Just something to call out. Perhaps we can handle that in a separate PR/Issue. |
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
lib/debug/ProfilingPlugin.js
Outdated
@@ -137,7 +137,9 @@ const createTrace = outputPath => { | |||
callback(); | |||
}); | |||
// Tear down the readable trace stream. | |||
trace.destroy(); | |||
if (trace.destroy) { |
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 wonder if destory
is the correct way to end the stream. Isn't trace.push(null)
the better way?
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 tried this and it seem to work (also on node.js 6).
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 not sure if destroy
is the correct way to end the stream. It works for 8, but not for 6.
They don't happen for me on master... |
@sokra - i am running the following: git log -n 1
node -v
echo $CI
yarn test -- test/ConfigTestCases.test.js and i see:
The test seems to depend on: So I think it works on the CI but not locally for me? It's also a timeout, so maybe my machine is slower than yours (although I'm running on a 2017 macbook w/ maxed out RAM :) ) |
Fixes #7654
This fixes the node 6 warning I was seeing (see issue #7654). I'm not sure if there's a way to clean up the readable stream in node 6.
What kind of change does this PR introduce?
Prevents calling a method that doesn't exist in node 6
Did you add tests for your changes?
No. I tested manually by running this code against my codebase in node 6 (before and after making the change)
Does this PR introduce a breaking change?
I hope not.
What needs to be documented once your changes are merged?
I don't think anything really needs to be documented. This just prevents a warning from showing up in node 6.