allow webpack to find common watchers

[shamrin]

This change allows the following to work when bundling the application with webpack:

import {Program} from "witheve";
let prog = new Program("example");
prog.attach("system");

Without it I get the error in browser console:

Uncaught Error: Unable to attach unknown watcher 'system'.
    at Program.attach (main.js:17351)
    …

[joshuafcole]

Hey @shamrin,
This approach unfortunately breaks SystemJS loading. It also only works for first party watchers. I'm tempted to say that we could wrap the require in a check (using if(Watcher.get(id)) { ... }), but unfortunately what works for webpack does not work for browserify which in turn does not work for requireJS and so on. To avoid special casing for each, I'm trying to keep the runtime module as bundler-agnostic as possible.

My thinking for embedded Eve programs was that users would manually require the watchers they're interested in using either their bundler's configuration file or an explicit import in their app. That said, I think it'd be a fantastic idea to build a starter framework targeting other bundlers such as webpack (just like eve-starter targets SystemJS) to make it easier for users more familiar with those tools to start working with Eve. If you're interested in spearheading one of those, I'd be happy to assist you in any way I can.

Additionally, the floor is definitely open for discussion around this point. If there's a good cross-tooling way to make Eve dependency management easier, I'm all ears!

[shamrin]

@joshuafcole How does it break SystemJS loading? I've just tried eve-starter and it seems to work fine:

cd Eve
git checkout webpack-watchers
npm link
cd ../eve-starter
npm link witheve
npm run build
npm start -- -p 1234

How can I see the breakage?

It also only works for first party watchers.

Could you show an example? I was under impression that this patch doesn't really change anything. It's simply a hint for webpack, so that it would bundle all of watchers/*.js.

I agree about Eve starter project for webpack. I'll try creating one.

[joshuafcole]

How can I see the breakage?

Forgive me, I had pause on caught exceptions enabled still from prior debugging.

Could you show an example? I was under impression that this patch doesn't really change anything.

Right, it's not that it changes anything compared to what the runtime previously did, it's that it isn't as flexible as the watcher loading system in the eve-starter. The latter has multiple search paths, which means users can create watchers outside of the Eve runtime repository, whereas this can only load the first party watchers present in that one directory. That means a user is still on the hook for manually requiring third party watchers or watchers she's created herself.

Since it doesn't explicitly break anything and is a relatively small change, I'm considering pulling it in anyway, even if it is webpack specific. While we think on it, if this PR effects you in any way, please voice your opinions.

[shamrin]

I'm also not very satisfied with require(`../watchers/${id}.js`) hack.

In addition, I still don't understand watcher loading system advantages. For example, eve-starter doesn't have custom watchers that you then .attach to a program. It looks its advantages are theoretical at this point. Would you consider throwing watcher loading system away in favour of simple modules? We could do use them something like this:

import {Program, html} from "witheve";
let program = Program("test");
program.attach(html);

Or:

import {Program} from "witheve";
import {html} from "witheve/watchers";
let program = Program("test");
program.attach(html);

Attaching second- or third-party watcher becomes trivial. You would just import the necessary module.

As a bonus, it would solve webpack problem, because watcher modules would be externally exported.

[joshuafcole]

I still don't understand watcher loading system advantages.

The primary advantage is that it doesn't require a module loader at all. That is, if you simply concatenated the watchers into your bundle all programs would still have access to those watchers. It also lets the editor know about third party watchers as a side benefit. Without it, you'd always have to write some javascript or do some configuration for the editor to find them.
These benefits aren't huge, but they are nice to have.

For example, eve-starter doesn't have custom watchers that you then .attach to a program. It looks its advantages are theoretical at this point.

While eve-starter doesn't currently contain custom watchers, it does support including them using the --include path/to/watchers flag. In the near future we'll add a couple of example watchers to the repository to make that more clear.

Would you consider throwing watcher loading system away in favour of simple modules?

Yes, but I'm not sure the registry is actually the core problem here. I think -- as you've identified -- that we need to explicitly export the first party watchers if they're going to live within the runtime module. That way it's a much simpler process to require them in whatever manner is most convenient for bundling. We've also discussed splitting them out into their own modules either separately or together. I think we'll want to move in that direction eventually, where you just use NPM to manage your watcher dependencies and require the ones you need.

If you have specific arguments against keeping the registry though (rather than also exporting the watchers), now's a good time to discuss those. We have no commitment to that strategy until we release 0.3.

[shamrin]

Yes, registry is not the core of the problem. I will add explicit export of first party watchers to this PR.

If you have specific arguments against keeping the registry though (rather than also exporting the watchers), now's a good time to discuss those.

Ok! Without the registry (but with plain modules):

1. We wouldn't have this discussion in the first place :)
2. There would be an explicit link between import and attach. You would just import the module, than attach the module to the program. Now you need to import/require (implicitly or explicitly), then attach by name. And make sure you use the correct name when attaching: import "third-party-watcher"; program.attach("third-party-watcher"); (and also when writing the module)
3. No need to maintain explicit watchers export list two ways to load watchers.
4. No need for extra watchers-handling code in eve-starter.

The primary advantage is that it doesn't require a module loader at all.

I would love if we could do without module bundling/loading tools. However, we already need the module system to load witheve module, even in eve-starter repo. Why not use the module loading system for watchers as well?

It also lets the editor know about third party watchers as a side benefit. Without it, you'd always have to write some javascript or do some configuration for the editor to find them.

We already need to write some javascript to require necessary modules and do some configuration (--include path/to/watchers option).

P.S. Please avoid splitting stuff into NPM modules if you can. Let's not repeat lodash module madness. If we want to save some bytes and not load unused watchers, we could use dynamic imports in SystemJS or tree-shaking in Rollup or Webpack.

[shamrin]

I've added explicit export of first party watchers. Also I've added related PR: witheve/eve-starter#9.

(I could remove watcher loading system separately, if we would agree to do it.)

[shamrin]

@joshuafcole Could you please chime in on the latest state of this PR? It seems to be incontroversal. I haven't touched watcher loading system in any way.

[joshuafcole]

Hey @shamrin, sorry for the long delay, I've been a bit wrapped up in revamping our web presence since last week. This all looks pretty good. One small change I'd recommend is to export all the watchers together from src/watchers/index.ts, and then export that through the main index to prevent cluttering. Once that's done, this is good to go.

I want to take the time to carefully read and respond to your other post, but fixing these presence issues is very urgent. I promise that no decisions will be made without reading and responding first!

[joshuafcole]

Hey @shamrin,

Just wanted to follow up to see if you were still interested in pursuing this? If not, I can make the last few touchups and get it merged instead. I think it's a good idea which will help our non-systemJS users significantly.

[shamrin]

Yes, I'm still interested. I'll take a look at it this weekend. Sorry for the long delay!

[joshuafcole]

Not a problem! Let me know if there's anything I can do to help.