[WIP/RFC] Warn when a fs call cannot be evaluated #587

fathyb · 2018-01-19T11:20:17Z

This PR will warn instead of throwing when a fs call cannot be evaluated.

Remarks :

I added a commit to create a logger singleton, we might want to wait for Logger singleton #462 to land first and then remove the commit.
It warns using a code frame, maybe it's too much, waiting for your feedback for that
The warning will not be propagated to the browser console
If the call cannot be evaluated then the fs module will be required as an empty module (Uncaught TypeError: fs.readFileSync is not a function on the runtime)
See the nodeNotEvaluated var in fs.js, I don't know if it's elegant
Should we handle the case where the filename is not a string? (eg. readFileSync({notAString: true}))

TODO:

warn
add a sweet emoji right before the start of the warning (Add a warning emoji ⚠️ to the logger #588)
add tests
needs Logger singleton #462 for the logger

codecov-io · 2018-01-19T11:24:05Z

Codecov Report

Merging #587 into master will increase coverage by 4.9%.
The diff coverage is 85.71%.

@@            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

Impacted Files	Coverage Δ
src/Logger.js	`96.96% <66.66%> (+0.14%)`	⬆️
src/WorkerFarm.js	`93.22% <87.5%> (+0.23%)`	⬆️
src/visitors/fs.js	`87.5% <88.23%> (+1.97%)`	⬆️
src/worker.js	`90.9% <0%> (-9.1%)`	⬇️
src/assets/CSSAsset.js	`82.35% <0%> (-7.36%)`	⬇️
src/utils/installPackage.js	`93.75% <0%> (+3.12%)`	⬆️
src/assets/LESSAsset.js	`100% <0%> (+5%)`	⬆️
src/assets/SASSAsset.js	`100% <0%> (+5.26%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fddfdb9...ee26483. Read the comment docs.

DeMoorJasper · 2018-01-19T15:59:39Z

src/Logger.js

 class Logger {
+  static forAsset(asset) {


Won't this bug the logger if multiple loggers exist?
Mainly like glitches with the status

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).

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)

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]

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.

fathyb · 2018-01-19T16:30:32Z

test/integration/fs-options-non-evaluable/index.js

@@ -0,0 +1,5 @@
+const dir = __dirname


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 😕

devongovett · 2018-01-31T06:43:44Z

src/visitors/fs.js

@@ -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));


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.

devongovett · 2018-01-31T06:46:23Z

src/Logger.js

 class Logger {
+  static forAsset(asset) {


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.

devongovett · 2018-01-31T06:47:42Z

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.

fathyb · 2018-01-31T21:45:14Z

I pushed a new commit according to the review and added a code frame where fs.readFileSync throws.

devongovett · 2018-02-05T05:03:46Z

Thanks! Cleaned it up just slightly to take advantage of the existing generateErrorMessage function on JSAsset. That will also provide a non-highlighted code frame for CLIs that don't support color.

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.

fathyb mentioned this pull request Jan 19, 2018

🐛 d3 import: Could not statically evaluate fs call #298

Closed

fathyb force-pushed the fix/fs-call-eval branch 2 times, most recently from 6ba2512 to 188a202 Compare January 19, 2018 15:44

DeMoorJasper reviewed Jan 19, 2018

View reviewed changes

fathyb commented Jan 19, 2018

View reviewed changes

fathyb mentioned this pull request Jan 23, 2018

TypeScript import '@types/handlebars'; leads to "Cannot resolve dependency '@types/handlebars'" #615

Closed

devongovett added the 🙋‍♀️ Feature label Jan 26, 2018

devongovett requested changes Jan 31, 2018

View reviewed changes

fathyb force-pushed the fix/fs-call-eval branch 2 times, most recently from 765d6c4 to 63a80a0 Compare January 31, 2018 21:40

fathyb force-pushed the fix/fs-call-eval branch from 63a80a0 to ac391e5 Compare January 31, 2018 21:52

fathyb and others added 4 commits February 4, 2018 20:59

Improve fs error reporting

2d11b20

Don’t log when warming workers

78e0a1f

Make pretty errors in logger.warn

cb3e72d

Generate fs warnings using Asset.generateErrorMessage

ee26483

devongovett force-pushed the fix/fs-call-eval branch from ac391e5 to ee26483 Compare February 5, 2018 04:59

devongovett approved these changes Feb 5, 2018

View reviewed changes

devongovett merged commit 6d23efd into parcel-bundler:master Feb 5, 2018

devongovett pushed a commit that referenced this pull request Oct 15, 2018

Warn instead of error when an fs call cannot be evaluated (#587)

d06c709

devongovett pushed a commit that referenced this pull request Oct 15, 2018

Warn instead of error when an fs call cannot be evaluated (#587)

d6b9a06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP/RFC] Warn when a fs call cannot be evaluated #587

[WIP/RFC] Warn when a fs call cannot be evaluated #587

fathyb commented Jan 19, 2018 •

edited

codecov-io commented Jan 19, 2018 •

edited

DeMoorJasper Jan 19, 2018

fathyb Jan 19, 2018

DeMoorJasper Jan 19, 2018 •

edited

fathyb Jan 19, 2018

devongovett Jan 31, 2018

fathyb Jan 19, 2018 •

edited

devongovett Jan 31, 2018

devongovett Jan 31, 2018

devongovett commented Jan 31, 2018

fathyb commented Jan 31, 2018

devongovett commented Feb 5, 2018

[WIP/RFC] Warn when a fs call cannot be evaluated #587

[WIP/RFC] Warn when a fs call cannot be evaluated #587

Conversation

fathyb commented Jan 19, 2018 • edited

codecov-io commented Jan 19, 2018 • edited

Codecov Report

DeMoorJasper Jan 19, 2018

Choose a reason for hiding this comment

fathyb Jan 19, 2018

Choose a reason for hiding this comment

DeMoorJasper Jan 19, 2018 • edited

Choose a reason for hiding this comment

fathyb Jan 19, 2018

Choose a reason for hiding this comment

devongovett Jan 31, 2018

Choose a reason for hiding this comment

fathyb Jan 19, 2018 • edited

Choose a reason for hiding this comment

devongovett Jan 31, 2018

Choose a reason for hiding this comment

devongovett Jan 31, 2018

Choose a reason for hiding this comment

devongovett commented Jan 31, 2018

fathyb commented Jan 31, 2018

devongovett commented Feb 5, 2018

fathyb commented Jan 19, 2018 •

edited

codecov-io commented Jan 19, 2018 •

edited

DeMoorJasper Jan 19, 2018 •

edited

fathyb Jan 19, 2018 •

edited