Bring Scribe Common into Scribe

[rrees]

Addresses #268 in the simplest way possible

[hmgibson23]

I'm all for this ... but @theefer, @robinedman and @cutandpastey might have some comments about it.

[theefer]

Are you going to update all plugins to use helpers from scribe instead of scribe common? If planned for a later step, it might be worth doing at least one to prove it's doable (incl. running the plugin tests, bundling it correctly, loading it in, etc)?

As per @OliverJAsh's comments in https://github.com/guardian/scribe/issues/268, do you have any solution in mind to enforce version matching between plugin and Scribe? (npm's peerDependencies could work, I think, but Bower doesn't support them AFAIK. There may be hacky work around ways?)

I don't have any problem with this change as long as the problems people identified are considered.

[rrees]

@theefer I wasn't clear whether I needed to release Scribe first then check the plugins or whether it would be enough to override the bower_module locally and then see what happens.

As for version matching, I can't honestly say that I followed the meaning of the discussion. The thing that makes the most sense to my mind is to expose scribe.Node and scribe.Element on the scribe instance that you pass in to the plugin. Loading a separate object from a separate package doesn't make sense to me.

[theefer]

I wasn't clear whether I needed to release Scribe first then check the plugins or whether it would be enough to override the bower_module locally and then see what happens.

You may (?) be able to test things without releasing them using bower link to link to local versions, rather than addressing published ones. It's a bit of a faff, but probably less tedious than having to do micro-fixes releases if it's not working.

As for version matching, I can't honestly say that I followed the meaning of the discussion. The thing that makes the most sense to my mind is to expose scribe.Node and scribe.Element on the scribe instance that you pass in to the plugin.

I think the gist of it is that because these helpers will evolve (new ones, fixed ones, deprecated ones (?)), you will likely want to enforce some versioning: plugin A needs version N or later of the helpers to work (say because the helper it uses was added in N). Otherwise there is no way to tell if a plugin will work with whatever version of Scribe you may be running.

This is currently done by:

1. Having the master branch explicitly depend on a version (or later patch versions) of scribe-common (e.g. https://github.com/guardian/scribe-plugin-noting/blob/master/bower.json#L11)
2. Building that version that is known to work into the plugin distributable
3. Guaranteeing that later patch versions are backwards compatible (as per semver)

As I wrote this, I wonder if the solution isn't simply to stop doing 2 and stop building scribe-common into the plugin distributable. (I'm possibly/probably wrong, please @hmgibson23 and @robinedman correct me).

Instead, declare the dependency on whatever version of scribe-common you need in the bower.json of the dist branch, thus making it a "late-binding", in other words, expect that the requirejs environment the plugin is loaded in provides scribe-common. That way, Bower is used to enforce versions and force the user to resolve any version conflict explicitly.

This should solve the problem of having to re-build all plugins whenever scribe-common changes to avoid plugins overriding it in the requirejs context with an older version, which is the problem we saw before.

Note that I think this is very similar to what you're suggesting doing by putting the helpers into scribe, but it allows keeping them separate and updating them separately.

It sounds almost too easy to be true, so there may be caveats? Can anyone tell me why my idea is dumb and doesn't work please? :-)

Alternatively, if you still want to put the helpers back into scribe, it may be possible to do the same thing and add the required version of scribe as a dependency in the bower.json of the dist branch.

But it's all fiddly enough that I would personally trial any change on a small scale before committing to it.

[hmgibson23]

I can't see anything wrong with your potential solution @theefer. I'm slightly lost on all the versioning stuff myself. The only problem I can see is that we will have to rebuild all of the plugins anyway to get rid of whatever version of scribe common they have in them.

[theefer]

That's true regardless of the solution (scribe, scribe-common, other), isn't it? But at least should make your life easier from then onwards!

[rrees]

We need to talk IRL about this but I think having a separate library for your interfaces because you potentially want to make backwards incompatible changes is a sledgehammer to break a nut here. Using semantic versioning conventions on Scribe itself should be enough.

[theefer]

I agree that we shouldn't make backwards incompatible changes, unless using semver to denote it. I think we haven't done that so far, so that's cool.

We likely do want to maintain the ability to version for forward changes (you need at least version X of scribe/scribe-common). I think it can be done as described above using dependencies in the dist branch but I may have missed something.

It seems that the suggestion of not bundling the helpers with the plugins should hopefully solve most problems, and it works regardless of whether they live in scribe or scribe-common, so the team should be able to make the call where they want them, as long as everything else (tests, browserify, etc) still works :-)

[rrees]

I am going to implement the suggestion of expose Node and Element on Scribe, which would then allow me to rewrite the "bundled" plugins to use this instead of the source directly. This should be sufficient proof of concept to show how plugins might be ported.

[robinedman]

So I like this solution. Keeping it simple. :+1: from me.

For a change like this though, might be good to have another +1 too so we're all on the same page. @hmgibson23 ?

[hmgibson23]

It did fail the build though.

[rrees]

@hmgibson23 The power of Travis rescheduled build compels you!

[hmgibson23]

:+1: in that case