implement importNewModule

[tech4GT]

@jywarren this implements importNewModule functionality for both node and browser environments, in node it is as simple as sequencer.importNewModule(name,options) In browser the module maker must browserify the code before publishing it for frontend use. For example to use invert as a dynamically imported module it will look something like this

/*
* Invert the image
*/
function Invert(options, UI) {

  var output;

  // The function which is called on every draw.
  function draw(input, callback, progressObj) {

    progressObj.stop(true);
    progressObj.overrideFlag = true;

    var step = this;

    function changePixel(r, g, b, a) {
      return [255 - r, 255 - g, 255 - b, a];
    }

    function output(image, datauri, mimetype) {

      // This output is accessible by Image Sequencer
      step.output = { src: datauri, format: mimetype };

    }

    return require('../_nomodule/PixelManipulation.js')(input, {
      output: output,
      changePixel: changePixel,
      format: input.format,
      image: options.image,
      inBrowser: options.inBrowser,
      callback: callback
    });

  }

  return {
    options: options,
    draw: draw,
    output: output,
    UI: UI
  }
}

if(this.module) {
  module.exports = Invert;
} else {
  sequencer['invert'] = {
    func: Invert,
    info: {
      "name": "Invert",
      "description": "Inverts the image.",
      "inputs": {
      }
    }
  };

Signed-off-by: tech4GT varun.gupta1798@gmail.com

[tech4GT]

@jywarren Once you approve we can work through the documentation for this since it is gonna be very important that we have good documentation on this since this is a bit tricky.

[jywarren]

Hmm, i see. In principle we should have the same formatting for both, but I understand what you mean -- we can't use any non-browser friendly code if we're just loading on the fly. What's an example of something that wouldn't work, to clarify?

Thanks!!

[tech4GT]

@jywarren something where we require get-pixels or any other module won't work

[tech4GT]

The invert module would also not work like this unless we import es6 style

[tech4GT]

@jywarren this gives me an idea that we should try to move our codebase to es6 over time

[jywarren]

@jywarren something where we require get-pixels or any other module won't work

Hmm, this may be a non-starter until we can figure out a way through this. We wouldn't want our modules to start fragmenting into node and non-node -- that's one of the core benefits of the whole library, you know?

Let's brainstorm a bit. Are there replacements for real-time require() in browser? How does, for example, the preview this module function work in npmjs.org? https://runkit.com/npm/lodash

Or, could we make getPixels available through a non-require function? Wrap it?

One more thing -- is there a reason we couldn't call this importModule instead of importNewModule? Simpler maybe!

[tech4GT]

@jywarren Actually One good thing is that this affect the existing system, so we can keep publishing our own modules bundled with the library
As far as other modules go I had an idea that modules can be distributed through npm no problem and they will work inside of node context and for frontend the module maker would have to browserify the code and distribute the script via a cdn

[tech4GT]

Actually If we see the use cases, inside a frontend only project which is not being built the cdn script can be used. If somebody is using the module in node then he can use it via npm. Also if somebody is building a frontend project with any service and downloading the package via npm, his build tool will build the module as well and the require will be replaced then...
That's what I thought

[jywarren]

Looking into options here, but i'm worried about relying on something that's not so well proven: https://stackoverflow.com/questions/5168451/javascript-require-on-client-side

[jywarren]

What about wrapping get-pixels and a few others in a function locally so you don't have to rely on require? And if you want to do a module that really does need to require() something, then we say you need to browserify it.

[tech4GT]

@jywarren Actually One more thing, this is gonna be on the module maker's side. I mean we are not going to dynamically load our own module which is bundled with the source anyway.
What I am saying is that this is pretty open since different modules can use different solutions...

[tech4GT]

Basically what we are doing is that a dynamically loaded module is Stand-Alone and hence all the processing code needs to be inside the module. I fit uses get-pixels then it would have to include it with it's code. This also enables the developer to use any alternative to get-pixels module as well if they wish to

[tech4GT]

@jywarren Maybe we can make a few npm packages like get-pixels available inside the module locally...you are right so if the person wants to use an alternative or any other module then he needs to bindle all the code in a script

[jywarren]

ok, let me think about this a bit sorry i'm doing work on the live PublicLab.org server right now... will circle back! Also, maybe @ryzokuken has some thoughts on convenience and usability on this topic?

[tech4GT]

@jywarren Oh that is fine I'll pick some other issue for now and for this I'll just do the part to make get-pixels available locally in module

[tech4GT]

@jywarren Also I am making pixelManipulation API locally accessible, I hope that's fine?

[tech4GT]

@jywarren I came up with a refactor of the pixelManipulation API,
I am defining all the utility modules on the input object passed to the module inside of draw, so basically now we can say

// Inside Module
draw(input){
input.getPixels()
input.pixelManipulation(options)
}

[tech4GT]

@jywarren I just included all the apis we use in our modules on the input object so that any dynamically loaded module can use these, If any other piece of code is required it must be bundled with the module.

[jywarren]

OK, this is interesting! It is a breaking API change? What would the documentation look like to explain this with the importModule system? Maybe an example of one using only available utility modules. And a second showing how you'd have to browserify if you want to include more with require()?

[tech4GT]

@jywarren No I made sure this is not a breaking change!! I'll write up some documentation for this now, I was waiting for your approval only!! Great!

[tech4GT]

Also @jywarren Since we use the naming importString for importing a string into the sequencer, importModule feels like doing that for a module, hence I named it importNewModule

[tech4GT]

@jywarren Please have a look I added documentation on how to do a dynamic module.

[tech4GT]

@jywarren I included all the changes, also I reconfigured my IDE to the style you suggested so all the files will follow the convention in the future. Once we are done with this, we can add the required tests in #293 . Following this I want to complete meta-modules and then add our new dynamic loading functionality in the cli as well(Here I am thinking a way where user does sequencer --add-from-npm sequencer-invert and this downloads the dependency and calls loadNewModule on it)

[tech4GT]

@jywarren can you please have a look at this too

[jywarren]

oh wait, shouldn't this be not image-sequencer but a specific module?

[jywarren]

small typo here -- usingrequire - and it can be great to use ` characters as you did with loadNewModule, right?

[tech4GT]

Done!!

[jywarren]

OK, a couple tiny changes, but otherwise do you think this is good to go?

[jywarren]

Thanks, Varun!!!

[tech4GT]

@jywarren this is pretty much ready to go, after this is done first I will add tests for getStep which will close another big PR
Next Meta-modules and after that I'll make the modules dynamic(groundwork for which is already in place). This should complete most of the core work

[tech4GT]

So to summarize after the above mentioned things are done we close #262 #190 #200 #242 (the core part)!!! After all this let's have a major version bump and publish on npm

[jywarren]

OK, do you know why tests are failing? I think we are ready otherwise!

[tech4GT]

@jywarren must have been a bug with travis, i reopened the pr and now it is fine!!

[jywarren]

OK, done! Great work!!!

[jywarren]

OK awesome, so does this close #262 #190 #200 #242 ??

Exciting!

[tech4GT]

Yay!!

[tech4GT]

@jywarren Not all, But very less work is required now!! Rebasing the other PR over this!!

[jywarren]

super

…

On Wed, Jul 18, 2018 at 10:17 AM Varun Gupta ***@***.***> wrote: @jywarren <https://github.com/jywarren> Not all, But very less work is required now!! Rebasing the other PR over this!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#294 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ0-R6_beuZMykXKbhHyMOBCLz5P5ks5uH0NWgaJpZM4UuEhT> .

[tech4GT]

@jywarren I am adding tests for #293 now after which we can merge that too!! Can you please also take a look at #297