shout icon indicating copy to clipboard operation
shout copied to clipboard

[Discussion] shout/client dependency management

Open MaxLeiter opened this issue 8 years ago • 16 comments

At the moment a lot of the dependencies are outdated (Handlebars is distributed as 2.0, but the current version is 4.0.4).

Personally, I'm a fan of making the client it's own repository and adding it as a submodule to erming/shout.

MaxLeiter avatar Dec 22 '15 04:12 MaxLeiter

Hi @MaxLeiter!

You mention 2 different things in your issue: browser dependency management and server/client separation. Regarding the second point, we might get there someday indeed, but we are not quite there yet for many reasons (some of which are consequences of the roadmap: https://github.com/erming/shout-roadmap).

However, I very much agree with your first point, and that would be great if we can do something about this. Specifically, I'd love to see all our dependencies be managed by npm. I know bower is also an alternative for client-side dependencies, but lacking a proper registry among other things, it is a poor choice when you have npm around!

Do you think you could help with this? That would be very helpful and we would definitely be grateful! :-)

The way I see it can be split in N steps:

  • [ ] Discuss to make sure we are on the same page, list affected dependencies, find immediate caveats.
  • [ ] Move existing dependencies to the package.json (npm) file, in the version Shout currently uses
  • [ ] Use the simplest/dumbest solution to replicate current state regarding concatenation/distribusion of libs.
  • [ ] Come up with a nice integration solution (grunt? browserify?). Discuss advantages/drawbacks, think of how this alters releasing/deploying Shout, ...
  • [ ] When the skeleton of this is ready, start migrating versions and update code accordingly
  • [ ] Document things to help future us/future devs to keep things clean

What do you think of this? My guess is that we can quickly get to the first three stages together, and then the other ones will be an ongoing effort :-)

astorije avatar Jan 09 '16 19:01 astorije

Just thought I'd add my 2p. I think using npm for the client dependencies is the way to go. Its the way things are generally moving and means that we don't add another set up command.

AlMcKinlay avatar Jan 09 '16 19:01 AlMcKinlay

Yes! :-) Feel free to comment on potential consequences you can think of! Would be great to start moving the client-side deps to npm as soon as possible (again, in the current version to not break anything, it's the first mandatory step to start upgrading stuff when necessary!).

astorije avatar Jan 09 '16 19:01 astorije

Well, obviously the issue with getting them via npm is that the way the dependencies work won't work for it (because currently we just concat and minify everything in the libs folder). I guess a couple of options would be:

  1. Have a file that requires all the necessary libraries (using browserify, or ES6 modules with babel or even requirejs) which would then have a grunt task to create the exact same minified libs source as we have now
  2. Move all the client code to modularised code (using browserify, ES6 modules etc). Then you can just require the dependencies in the modules that need them, and for just now (until HTTP2 is standard) we can concat and minify the whole thing like that. (this would be my preferred option, but obviously the most work. I have done this on another project before, and it's not fun, but it feels great at the end)

What do you think?

AlMcKinlay avatar Jan 09 '16 20:01 AlMcKinlay

What about this: for proof of concept/MVP, a simple npm script (like these ones) that does the minimal job. Then we can get all fancy if we want, but again, the primary goal here is to move current libs out of the repo and into the package.json.

That way we can still do things step by step (updated steps above) and improve over time.

astorije avatar Jan 10 '16 23:01 astorije

We could probably put the require's in client/js/shout.js, and for installing the dependencies we could use the prefix flag in order to handle the clients.

@astorije by documentation do you mean comments in the code, detailed commit messages, or something else?

MaxLeiter avatar Jan 11 '16 01:01 MaxLeiter

Hi @MaxLeiter,

@astorije by documentation do you mean comments in the code, detailed commit messages, or something else?

Regarding the last bullet point? Well, comments in the code when it makes sense and details commit messages, according to our CONTRIBUTING file, are always in order :P

This time though, I meant once we have adopted a strategy, write it down somewhere so that new contributors are not lost. This totally depends on the previous steps, and I would not worry about that for now.

We could probably put the require's in client/js/shout.js, and for installing the dependencies we could use the prefix flag in order to handle the clients.

I didn't know about this. Really, in phase 1, the simpler the better, even if it's not perfect. First goal is to move the deps from the repo to the package.json, I don't care if there is a nasty script that consolidates them all at that stage.

@MaxLeiter, would you be OK to work on the first 2 steps? Step 1 is really list the dependencies and describe briefly what needs to be done for step 2, which is the actual move. I'm pretty sure it's straightforward but I can't commit to it :-)

Any help on this would be great, you'd certainly do everyone a favor here!

astorije avatar Jan 13 '16 05:01 astorije

Dependencies:

Name Version shout uses Latest version npm (Y/N)
Favico.js 0.3.5 0.3.9 Y
handlebars 2.0.0 4.0.5 Y
jquery 2.1.1 2.2.0 Y
moment 2.7.0 2.11.1 Y
NotificationJS ? ? N
Socket.io 1.0.6 1.4.4 Y
string.contains.js ? ? N
stringcolor.js 0.2.0 0.2.0 N
URI.js 1.14.1 1.17.0 Y

stringcolor.js was made by @erming, and presumably string.contains.js was as well (no license/info/etc), but its a simple function and easy to maintain/move/whatever we choose to do.

Notification JS is a shim not hosted on npm, however it is on bower. Doesn't seem to be updated often so could probably be a shipped dependency (i.e. not handled by a package manager). On my fork I've begun work on migrating the dependencies over.

MaxLeiter avatar Jan 14 '16 01:01 MaxLeiter

This is great, you're off to a good start @MaxLeiter!

For Socket.io, the current package.json mentions a ~1.0.6. Although this is, I believe, used by the server, if client and server can stay in sync on that, that's a great!

I agree with you that what's not on npm right now should not be taken care of. As mentioned, this is the early work on client-side dependencies and, frankly, if at the end of this we mark the path with one client dep correctly managed and up-to-date, then that's a win! :-) For the 3 deps you mentioned, we'll see when we get there.

Good to know you are making progress on this, and let us know if you run into any trouble.

astorije avatar Jan 14 '16 07:01 astorije

Hey @MaxLeiter, how are you doing with this? :-)

astorije avatar Jan 19 '16 05:01 astorije

Sorry I've been busy lately; I plan to make more progress tomorrow.

MaxLeiter avatar Jan 19 '16 05:01 MaxLeiter

I can't say I don't understand that :-) No worries, whenever you can.

astorije avatar Jan 19 '16 06:01 astorije

This applies to css too, bootstrap and font-awesome are available via npm.

xPaw avatar Jan 20 '16 18:01 xPaw

Correct, let's ship the JS stuff with npm first, and open a second PR with the rest later. Small PRs, small commits :-)

astorije avatar Jan 21 '16 04:01 astorije

Finally found some time to work on this :)

Now comes some discussion: I feel the best way to 'duplicate' what's currently happening is to use client-side Requires with Browserify. The main reason I support Browserify over other module-loaders is that it concatenates the dependencies together (leading to a setup similar to what currently exists with libs.min.js).

Any other suggestions as to what to use instead of Browserify?

MaxLeiter avatar Jan 27 '16 00:01 MaxLeiter

So, in the long term (when we have the client code modularised and including the dependencies themselves rather than all the libraries in 1 file separately) I would want to use Rollup because it removes code that we don't use, decreasing the size of the whole client code.

Is it possible to use Rollup without the removing code xode for this bit, because then we wouldn't need to change what we use.

AlMcKinlay avatar Jan 27 '16 06:01 AlMcKinlay