add requireFseventsCallback option

[vjpr]

First draft.

I wanted to make the smallest change possible.

I initialized var fsevents = {} to get around the 'use strict' restrictions of modifying uninitialised variables.

I chose a callback over an emitter because some of the options are updated depending on the result of FsEventsHandler.canUse() which require fsevents to have attempted to be required.

Not sure on the name of the option. Maybe its better as requireFseventsErrorCallback.

No tests at present but wanted to get feedback on approach first.

#599

@es128

[es128]

Yeah the name is clunky, but I don't have a better suggestion atm.

Approach LGTM.

[es128]

May as well put if (opts.useFsEvents) in front of this. It will add the advantage of sparing some initialization time for everyone else.

[vjpr]

Maybe renaming from init to tryRequireFsEvents? Or tryInstantiateFsEvents? tryUseFsEvents?

[es128]

initFsEvents

[vjpr]

canUse is used at the top-level scope, so maybe its better to initialise it in canUse?

Or I move the init function to here.

// Attach watch handler prototype methods
function importHandler(handler) {
  Object.keys(handler.prototype).forEach(function(method) {
    FSWatcher.prototype[method] = handler.prototype[method];
  });
}
importHandler(NodeFsHandler);
if (FsEventsHandler.canUse()) importHandler(FsEventsHandler);

---

Hmm, this becomes tricky, because we need access to those opts. I will attach the fsevents after we run the check in the constructor...

[vjpr]

• I have made the callback a Node errback which will report the resolve path of fsevents.
• I have added an option to configure the resolved path to fsevents. If a package is symlinked, it will not be able to find the root project's fsevents (without using --preserve-symlinks). Allowing this option will mean consumers can have a way to manually locate fsevents.
• I have moved importHandler inside FSWatcher.

[es128]

What just happened? Now it looks like kind of a mess.

What was so wrong with where you had it before aside from initializing the fsevents var with an empty object instead of leaving it undefined or some other falsy value?

[vjpr]

What was so wrong with where you had it before aside from initializing the fsevents var with an empty object instead of leaving it undefined or some other falsy value?

See #602 (comment).

[vjpr]

I'm not happy with splitting up the importHandler methods. Maybe we should move the other importHandler into the constructor. Or we just leave the FsEventsHandler methods on the class and don't do the canUse check.

[es128]

Sorry, I'm not understanding it right now. I'll need to try it locally when I get some time later to figure out why it couldn't have stayed simpler.

[vjpr]

Without moving the importHandler it broke, because FsEventsHandler.canUse was being called in the top-level scope of index.js (when require('chokidar') is called) before opts are parsed.

Also, let me know if you would allow opts.fseventsPath.

The rationale is: if you npm link to webpack-dev-server (for example) for debugging (or a package that requires webpack-dev-server), unless you npm install fsevents in webpack-dev-server's dir, it will not use fsevents. And even if you do, your app would end up using two different instances of fsevents.

[coveralls]

Coverage decreased (-1.2%) to 97.201% when pulling 2c6a383 on vjpr:feature/599-fsevents-error into 3b1071a on paulmillr:master.

[coveralls]

Coverage increased (+0.009%) to 98.455% when pulling 839b0b7 on vjpr:feature/599-fsevents-error into 3b1071a on paulmillr:master.

[coveralls]

Coverage decreased (-1.2%) to 97.201% when pulling 2c6a383 on vjpr:feature/599-fsevents-error into 3b1071a on paulmillr:master.

[vjpr]

Just ran into this issue once again with a silent Error: Module did not self-register. message.

Its tough, because cpu spikes to 120%, which could be caused by something else in the app, but now I always have to find and then add console.log to that try/catch block.

With the current solution, if library devs (such as webpack-dev-server) don't implement the hook, then I am stuck, and we don't help anyone.

I think we really need an environment variable. I would set this env var permanently in my ~/.bashrc on macOS so I could always get notified if fsevents wasn't used.

[coveralls]

Coverage decreased (-0.1%) to 98.309% when pulling 1588e03 on vjpr:feature/599-fsevents-error into 3b1071a on paulmillr:master.

[coveralls]

Coverage decreased (-0.4%) to 98.02% when pulling ba928b0 on vjpr:feature/599-fsevents-error into 3b1071a on paulmillr:master.

[es128]

I haven't had time to review closely, but I'm sorry to tell you that I think as this PR grows it becomes less likely to be accepted. A note in the readme reminding you to npm rebuild if you switch node versions may be where we end up if we can't get to any other effective solution that is also simple and small.

I suggest you focus this down to the one simple solution that will have a positive impact on your quality of life relating to this problem, within the constraints I've already described. Perhaps that's going back to before the other changes you've made and only adding something catch block to print a message ONLY WHEN the user has set a specific env variable set. Yes, that only covers users savvy enough to set the env var, but I haven't seen any other proposal that won't cause collateral problems/complaints.

[vjpr]

I am very happy with just the env var personally. I will put together a fresh PR just for that.

…

On Thu, 11 May 2017 at 7:45 pm, Elan Shanker ***@***.***> wrote: I haven't had time to review closely, but I'm sorry to tell you that I think as this PR grows it becomes less likely to be accepted. A note in the readme reminding you to npm rebuild if you switch node versions may be where we end up if we can't get to any other effective solution that is also simple and small. I suggest you focus this down to the one simple solution that will have a positive impact on your quality of life relating to this problem, within the constraints I've already described. Perhaps that's going back to before the other changes you've made and only adding something catch block to print a message ONLY WHEN the user has set a specific env variable set. Yes, that only covers users savvy enough to set the env var, but I haven't seen any other proposal that won't cause collateral problems/complaints. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#602 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARLRUznKHFDx5sI4Z39MlJ-EHApnDLrks5r40lNgaJpZM4NUjkb> .