-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[WIP/RFC] Warn when a fs call cannot be evaluated #587
Conversation
Codecov Report
@@ Coverage Diff @@
## master #587 +/- ##
=========================================
+ Coverage 89.35% 94.25% +4.9%
=========================================
Files 64 64
Lines 2019 2036 +17
=========================================
+ Hits 1804 1919 +115
+ Misses 215 117 -98
Continue to review full report at Codecov.
|
6ba2512
to
188a202
Compare
src/Logger.js
Outdated
class Logger { | ||
static forAsset(asset) { |
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.
Won't this bug the logger if multiple loggers exist?
Mainly like glitches with the status
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.
Oh yeah, this is just to quickly showcase the feature. A IPC system is way better like you did in the PR, like said in the description I'll remove this commit and use your PR once it gets merged.
However I'm not sure if Node.js scrambles the logs when stdout
is shared across processes with child_process
(it sure does when each stdout
has it's own descriptor).
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.
Ow alright, thought this was another way to implement a singleton, guess i misunderstood.
About the workers stdout, I think it's best if we either don't use it or catch it on the master process and redirect it to the logger to make sure everything is streamlined to prevent console glitches (If we do catch stdout, we don't need to run it over ipc, but we can actually use stdout as an interface for workers to use the logger)
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 more for just using the IPC and not messing too much around stdout
(Windows + stdout
+ multiple processes don't blend well in my experience).
[bit off-topic]
And I really like the IPC idea, all my plugins are needing an IPC at some point and I ended up making my own using UNIX sockets. If we could streamline this and make it accessible to plugin developers it could really help.
[/bit off-topic]
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.
Jasper's logger changes have been merged to master, so the IPC should just work now. You can just use the Logger singleton within the worker and things will be transparently proxied.
@@ -0,0 +1,5 @@ | |||
const dir = __dirname |
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 is weird. This does not work (__dirname
is not found by the globals visitor) :
module.exports = require('fs').readFileSync(__dirname + '/test.txt', {
encoding: (typeof Date.now()).replace(/number/, 'utf-8')
})
While const dir = __dirname
works. I think it may be a babylon-walk bug, but I'm not really sure about that. I checked the Identifier
visitor and it is not called with the node, but it seems to be in the AST. I can't reproduce it outside of this test 😕
@@ -31,9 +37,37 @@ module.exports = { | |||
__dirname: Path.dirname(asset.name), | |||
__filename: asset.basename | |||
}; | |||
let [filename, ...args] = path | |||
.get('arguments') | |||
.map(arg => evaluate(arg, vars)); |
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.
Maybe instead of changing the logic to be a loop that exists early as below, you could just wrap this in a try...catch
block and do the logging in the catch. That way you'd also not need the nodeNotEvaluated
thing. Might clean up the code a bit.
src/Logger.js
Outdated
class Logger { | ||
static forAsset(asset) { |
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.
Jasper's logger changes have been merged to master, so the IPC should just work now. You can just use the Logger singleton within the worker and things will be transparently proxied.
This is awesome! The code frames are really nice I think. Definitely helps track down bugs. I could see how it might be annoying though if you know something cannot be evaluated on purpose (or in a node_module you don't own), and it keeps nagging you. |
765d6c4
to
63a80a0
Compare
63a80a0
to
ac391e5
Compare
ac391e5
to
ee26483
Compare
Thanks! Cleaned it up just slightly to take advantage of the existing Also needed to fix a couple things in WorkerFarm so that we don't log things when workers are warming up - caused duplicate messages to appear. |
This PR will warn instead of throwing when a
fs
call cannot be evaluated.Remarks :
fs
module will be required as an empty module (Uncaught TypeError: fs.readFileSync is not a function
on the runtime)nodeNotEvaluated
var infs.js
, I don't know if it's elegantreadFileSync({notAString: true})
)TODO: