Image-sequencer: Modular approach for adding modules: s.importModule(moduleName)

[tech4GT]

Right now the modules are hardcoded into image-sequencer(image-sequencer/src/Modules.js), that means any user wanting to use image-sequencer has to download all the modules(even if the user just wants to use 1 or 2) so what i am thinking is that what if we make this dynamic.

We publish modules as separate npm packages that can be plugged into the sequencer(kind of like passport.js does it with passport strategies that can be plugged into passport). So the modules.js now becomes modules.json file and we add an add-module option to the cli allowing the user to dynamically modify their image-sequencer according to their needs.

We can have a basic set which will be included with image-sequencer itself and new modules can be installed via npm. This way we can also separate core and module contributions and also simplify the module contribution system(Anybody can then create a plugin module for the project and publish it making it more open since no pr will be needed to the main project to add a module).

This will make image sequencer very modular and configurable.As far as the demo is concerned we can add all the verified modules(Modules that are added in the readme) and build it for browser and then serve that. We can also have a guide on how to correctly make a module for this project.
After we are done adding a new module for use will look something like this:

npm i image-sequencer
npm i sequencer-blur
sequencer --register-module sequencer-blur
sequencer -i "<image>" -s "blur" 

A PR will still be needed if the module creator wants to list his/her module to our list in the readme
Many famous node based projects already use this approach as i mentioned above passport.js(an authentication system) also does this as strategies for oauth providers like facebook and google

@jywarren what do you say, will this be in sync with your vision of the project for the summer?
I actually wanted to present to you all the major ideas that i had so that you can tell me which ones would be better and i mentioned the rest in my proposal but i also wanted to run this one by you.
So what say?

• [ ] Prepare a list of core modules that should be included with image sequencer itself
• [x] Implement one line operation with dynamic module
• [x] Modify modules.js to be dynamic through a json file so packages can be installed separately and loaded into image-sequencer
• [x] Move complex non-core modules into their own packages and separate them from the core modules
• [x] Build the cli around it to enable listing of all installed modules as well as installing and removing modules
• [ ] Add a list of verified by publiclab modules that independent module makers can send a pr to and have their module listed

[jywarren]

GitMate.io thinks the contributors most likely able to help are @ccpandhare, and @jywarren.

[tech4GT]

And yes, one more thing the current system would not be affected at all, so if we want to add modules as a part of the sequencer core itself that would also be possible through the pr system that we are following right now

[tech4GT]

@jywarren i may be thinking a bit too far ahead but we can even add features like listing all verified modules and installing verified modules from our cli something like

sequencer --install-module sequencer-blur
sequencer -s "sequencer-blur"
sequencer --list-all
sequencer --list-installed

This would use install the npm package and set it up for use

[jywarren]

Hi, i like this a lot. I do think we could maintain a set of "core modules" that cover extremely common image manipulations like blur, contrast, saturation, crop, and some others. Maybe whatever Instagram does :-P

https://github.com/publiclab/image-sequencer/issues/122 has some ideas for simplifying module submission that may be related.

I also want to suggest that there are LOTS of things that can be solved pixel-by-pixel using the Dynamic module, making new module submissions extremely easy in that way too.

https://github.com/publiclab/image-sequencer/blob/master/src/modules/Dynamic/Module.js

Could we have a "simple module submission" guide that automatically adds modules using just a dynamic "string expression" and "does the rest for you" to make new modules that rely on Dynamic? See how we could use that for things like contrast, and anything that can work pixel-by-pixel, you know?

[tech4GT]

@jywarren exactly, that's the idea, we can have a core set of modules on which other modules can also depend
We can surely have a basic module generation script which will create the whole module for us and all we have to give is one expression😁

[tech4GT]

@jywarren what do you say, is this the right way to go for summer, if yes then we would have to discuss and drop some existing stuff from my proposal since it is full and has no room...i would love your help on that😁

[tech4GT]

@jywarren also i think we would need to discuss this in a bit more detail, what would be the best means to reach you for a discussion?

[jywarren]

Hi, i'm sorry i'm a bit overloaded at the moment, do you think you could
ping @ccpandhare and others at @publiclab/reviewers and we could try to
talk through your points one by one?

On Mon, Feb 26, 2018 at 12:39 PM, Varun Gupta [e-mail]
wrote:

@jywarren https://github.com/jywarren also i think we would need to
discuss this in a bit more detail, what would be the best means to reach
you for a discussion?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/190#issuecomment-368584562,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJ4mKyOu1abRPLxhBqEtjR4Qw7jRNks5tYuxLgaJpZM4SRzeA
.

[tech4GT]

@jywarren sure, no prob.😁
Just let me know whenever we can work through this one and till then i will consolidate all these ideas into a neat todo-list👍

[tech4GT]

@jywarren and even better I'll link this issue in my soc proposal so we can go over this when we review that, will that be fine?😅

[jywarren]

well, why don't you start by listing out some of the ideas in a list here,
and we can break them out as we go into separate issues if we want. Thanks!

On Mon, Feb 26, 2018 at 3:55 PM, Varun Gupta [e-mail]
wrote:

@jywarren https://github.com/jywarren sure, no prob.😁
Just let me know whenever we can work through this one and till then i
will consolidate all these ideas into a neat todo-list👍

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/190#issuecomment-368646954,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJ-1FDWmeIJifzqpc_-nt8XjUC4o8ks5tYxoxgaJpZM4SRzeA
.

[tech4GT]

@jywarren sure that's totally doable, I'll start working on it😁👍

[jywarren]

super!

On Mon, Feb 26, 2018 at 4:02 PM, Varun Gupta [e-mail]
wrote:

@jywarren https://github.com/jywarren sure that's totally doable, I'll
start working on it😁👍

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/190#issuecomment-368649235,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJ0zugHTOUeiA18Ta0cHAALFuqG8-ks5tYxvsgaJpZM4SRzeA
.

[ccpandhare]

Yes, this is a much needed change. I'd be happy to help out! We can figure out a suitable structure. Great thinking, @tech4GT !

[tech4GT]

@ccpandhare thanks a lot, I'll consolidate the ideas discussed so far in a list and then maybe we can have a group discussion with @warren maybe on hangouts to finalize it
I really appreciate your support 👍😁

[ccpandhare]

👍

On Fri, Mar 2, 2018, 10:45 Varun Gupta [e-mail] wrote:

@ccpandhare https://github.com/ccpandhare thanks a lot, I'll
consolidate the ideas discussed so far in a list and then maybe we can have
a group discussion with @warren https://github.com/warren maybe on
hangouts to finalize it
I really appreciate your support 👍😁

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/190#issuecomment-369822814,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AT0xnBQx3jV85lFMPbYIwQrTYGqCe25fks5taNVbgaJpZM4SRzeA
.

[tech4GT]

@jywarren @ccpandhare please have a look i have added a list of basic ideas that we have put together right now, i would love to discuss this and figure this out even further, maybe we can have a realtime chat?

[jywarren]

Actually i wonder what @ryzokuken would have to say about this! @ryzokuken has done a lot of amazing modularity work on https://github.com/publiclab/plotsbot

Thanks!!!

[jywarren]

This is looking really great!!!

[ccpandhare]

Looks very nice!! :-)

[ryzokuken]

@jywarren I'll take a look, thanks!

[ryzokuken]

Now that I think about it, it seems like a really good idea. We might have a ton of differing opinions on how the final API should look (and that's a good thing, we could talk about it extensively), but I think there's no doubt everyone will benefit if the codebase becomes sufficiently modular.

That said, to me

any user wanting to use image-sequencer has to download all the modules(even if the user just wants to use 1 or 2)

isn't the actual motivation behind making the code modular. This is because if I recall correctly, image-sequencer is mostly used in Node (isn't it?), and file downloads aren't that big a problem. File downloads are indeed a problem in the browser, but we can look into more basic optimizations first (are we using Rollup to bundle our library code?)

Instead, modularization is important because:

1. It will allow the user to cherrypick modules
2. It will allow us to export a lot more modules by default (without worrying about clogging the core module with functionality)
3. Most importantly, it will make it much easier for us to reason about, and if needed, debug a code.

Let me know what you think about this, I'll love to talk out.

@tech4GT keep up the good work.

/cc @jywarren @ccpandhare

[tech4GT]

@ryzokuken that makes sense to me thanks a lot, actually right now i am working on putting this in my soc proposal.
I am currently thinking of doing it the way passport does it and we can improvise as we move along, what say?

[ryzokuken]

Cool. That sounds doable.

[tech4GT]

@jywarren how does this look, i have started working on a basic prototype for this😁

[ryzokuken]

Why not use a js file for binding modules instead? Require them alongside the core, and call a function on the core or something. Perhaps even the constructor of the core object could take an array of modules as an argument. This would make things easier IMO than parsing JSON and requiring them programmatically. (This is also how we do it in plotsbot).

[tech4GT]

@ryzokuken we do have a js file called modules.js for the binding purpose but i don't quite understand how will that work if we want to bind modules downloaded from npm without modifying the code, can you drop a code snippet maybe?

[ryzokuken]

@tech4GT I don't have any snippet ready, but will try something over the weekend.

[tech4GT]

@ryzokuken will that be more efficient, coz if we are just storing module details then why not just save as json and require the json itself for all the info?

[ryzokuken]

Wouldn't that be synchronous and depend a lot on disk I/O time (not fast enough)?

I mean, I don't think the config would be too big. Why not use a plain JS object and pass it into a function instead of parsing a JSON file for the same?

I hope you get my point.

[tech4GT]

I might be wrong here but how will we modify this js object and persist this change for when the user runs the service the next time, since we are allowing plugins we would need the change in plugins to persist but the changes to the ja object will get lost as soon as the scope is destroyed , does that make any sense?

[tech4GT]

We can just write the final state of this object to a file and that is what i was thinking with the json

[tech4GT]

@ryzokuken @ccpandhare @jywarren i have made a little demo here https://github.com/tech4GT/image-sequencer/tree/modular please do have a look
current workflow from CLI is

./index.js --bind Brightness

This binds the module to sequencer and allows the user to use it with its name in lowercase

[tech4GT]

[jywarren]

mostly used in Node

Actually at the moment it's probably mostly browser-based, FWIW, with an option to run things in Node. But we don't have a big user base or anything, so we're trying to stay consistent and flexible!

[jywarren]

Sorry, just catching up here -- for on-the-fly loading of modules in the browser context, what does this look like? Would we say, include the core library, and then add a <script> include for each of the external modules you'd like to use? Or would we have the core module fetch and initialize remote modules (via npmjs.org -- is this possible/advisable?)

Thanks!!

[tech4GT]

@jywarren since this was an idea for the summer i actually didn’t think about the browser side, I have thought a solution for node but now that i think about it, we can add a module registration feature in the core image-sequencer object itself so that a module can be added through an external script in the browser, so a module will link itself to core sequencer using ImageSequencer.bind(), do you want me start working on a demo for this?

[tech4GT]

an external module will have a index.js which will run something like this

/*create the module object here*/
ImageSequencer.bind(‘module-name’)

[jywarren]

Just updating here that we talked a little about this being sequencer.importModule(moduleName) rather than bind() -- for clarity! Awesome!

[jywarren]

Actually once this is completed, we can refactor how modules are loaded in, here: https://github.com/publiclab/image-sequencer/blob/2abf0fae0967342153ba6098d4af056d5873b538/src/ImageSequencer.js#L40

to iterate through that list and use importModule() for each one.

[tech4GT]

Started core work on this in #294

[tech4GT]

closing this now!!