guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.5k stars 245 forks source link

Refactored require paths for plugin compatibility #313

Closed cutandpastey closed 9 years ago

cutandpastey commented 9 years ago

As mentioned within https://github.com/guardian/scribe/issues/311 requiring immutable.js with:

'immutable/dist/immutable'

breaks the https://github.com/guardian/scribe-plugin-noting test suite. I've re-jigged the require paths so that this should no longer be a problem. As I don't work with other plugins would be good to know that this doesn't break anything else @robinedman @theefer @rrees? Test suite run's fine on my local.

rrees commented 9 years ago

At the moment none of the plugins are depending on immutable except the core code so it should be fine.

theefer commented 9 years ago

Did you check that you can still run the release scripts after this change?

theefer commented 9 years ago

Also I'm confused why we can't use the paths config to point to the right place, which I thought was what you did in your fix.

cutandpastey commented 9 years ago

Yeah, I changed the paths config in test/app/index.html within the noting plugin. I added the fix here as I assumed that https://github.com/guardian/scribe/issues/311 would apply to other plugins.

theefer commented 9 years ago

Ah OK I understand. So it sounds like you've fixed the problem, which only existed in the noting plugin? @rrees said no other plugins depend on immutable, so there shouldn't be any other problem?

It would seem inconsistent to use a path dependency for immutable-js when the other Scribe dependencies (lodash-amd, and html-janitor here which I think is for a plugin) use a configured path. In the current state of things, using Scribe implies configuring the paths for any libraries it uses. Given the mess required to get things hooked up (dist vs source, installed vs link, etc), it may be best to stick to that for now.

That said, in the future I'd strongly suggest looking into a better way to manage dependencies. jspm is working amazingly well for both media-service and workflow :-)

cutandpastey commented 9 years ago

Well, any plugin which depends on scribe#master will have this issue util we update thetest/app/index.html file within that repo. From a brief look there's these two:

but it also means that any plugin we want to update to use scribe's master branch will suffer the same problem and it's not immediately apparent what breaks the test suite from the debug output.

As far as I can see this causes no issues with scribe but will prevent further issues down the line which for me is a win, not sure how @robinedman @rrees and @hmgibson23 feel about this.

In terms of the future, it seems pretty trivial to ship the compiled source unless I'm missing something?

rrees commented 9 years ago

@theefer add an issue for looking at the conversion for JSPM, its where we want to go after all

cutandpastey commented 9 years ago

Also we do ship compiled code. It's just depending on scribe#master that doesn't. My bad.

theefer commented 9 years ago

In terms of the future, it seems pretty trivial to ship the compiled source unless I'm missing something?

Bundling a package with its dependencies causes a lot of issues:

So we could compile Scribe but exclude the dependencies (lodash, immutable), but then it's exactly the same problem of configuring where to load them from.

Really the way to avoid problems is to delay the bundling until as late as possible; ideally as part of the app bundling itself. In other words, we want the full dependency graph unfolded, from the app down to its dependencies and their dependencies (rather than any intermediate bundle). As a bonus, that means we're running everything from source (even better than source maps!), which helps development.

In practice, it's really, really hard to do this with Bower, because there is so much fiddly manual setup (exactly the things you're highlighting here, with so many paths having to be set everywhere).

NPM natively doesn't really help much either, because it's duplicating libraries at all levels, which is fine (ish) on the server-side, but we eventually need to flatten all versions (as Bower does) as we said we can't really have duplicates on the frontend. Also, NPM/browserify is very centered around CommonJS, and even though there are transforms and things you can inject to get AMD support (not sure about globals), it remains difficult to put in place deep down in your dependency graph.

That's where jspm comes in. It relies on NPM and Github as sources of structured packages, but then handles all the dependency configuration, format management/transformation, etc. Suddenly it's really easy to depend on new packages, regardless of where they come from or their format, and to use them without having to write a lot of config paths. And it also lets you bundle a dependency graph together, optionally with arithmetic to add/remove/isolate subtrees, and to run your app off the bundle instead of the sources, without having to change a single line of code manually (no changing of import scripts, etc).

Note that a lot of people have been debating this recently, including a long whiny and interest post on Medium, whose author unfortunately desperately wants to try and contrive Bower to do the job, for some reason. Still well worth a read, as what he describes is almost exactly what jspm provides.