search

broccolijs / broccoli-funnel

MIT License
66 stars 37 forks source link

Refactor symlink code and add commentary and a better human error. #80

Closed nathanhammond closed 7 years ago

nathanhammond commented 7 years ago

This is significantly more verbose but doing algebraic reduction on this in the past has already resulted in at least one bug.

I've added tons of commentary to make it clear for future travelers (read: me in six months).

stefanpenner commented 7 years ago

This looks like it will regress performance, please be mindful of the original implementations goal of mitigation of unneeded disk IO.

Please provide a failing test for issue, so we can identify exactly what needs to be changed.

nathanhammond commented 7 years ago

Since I managed to get a reproduction without a good test case yet, here is a test that has different behavior in 1.0.6 and 1.0.7: (git clone && npminstall && npm test)

     The Broccoli Plugin: [Funnel] failed with:
  Error: ENOENT: no such file or directory, lstat '/Users/nathanhammond/Repos/broccoli-funnel/tmp/funnel-output_path-CYpaNO7K.tmp'
      at Error (native)
      at Object.fs.lstatSync (fs.js:839:18)
      at symlink (node_modules/symlink-or-copy/index.js:60:26)
      at symlinkOrCopySync (node_modules/symlink-or-copy/index.js:55:5)
      at node_modules/broccoli-plugin/read_compat.js:58:9
      at tryCatch (node_modules/rsvp/dist/rsvp.js:538:12)
      at invokeCallback (node_modules/rsvp/dist/rsvp.js:553:13)
      at publish (node_modules/rsvp/dist/rsvp.js:521:7)
      at flush (node_modules/rsvp/dist/rsvp.js:2373:5)

  The broccoli plugin was instantiated at: 
      at Funnel.Plugin (node_modules/broccoli-plugin/index.js:7:31)
      at new Funnel (index.js:56:10)
      at Context.<anonymous> (tests/index.js:339:15)

The error is "correct", but:

nathanhammond commented 7 years ago

This looks like it will regress performance, please be mindful of the original implementations goal of mitigation of unneeded disk IO.

The only difference in terms of IO effort I can identify is no short-circuiting on (inputPathExist && !fs.existsSync(this.outputPath)) which instead gets invoked each time. Adding back in a guard for that actually makes it less correct as I believe we no longer would rimraf the broken symlinks.

The remaining code has no additional invocations of anything as a consequence of this change. The increased verbosity is simply to make the code tractable. It is now much easier to confirm via manual review that we do the minimum amount of work necessary for each of the six scenarios–the situation for each is described inline.

nathanhammond commented 7 years ago

Test case up, ready for review/merge.

stefanpenner commented 7 years ago

ah the bug was actually broken input, and we didn't detect an error fast. Sounds good!