Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugins encapsulation #6

Closed
TrySound opened this issue Jul 2, 2015 · 31 comments
Closed

Plugins encapsulation #6

TrySound opened this issue Jul 2, 2015 · 31 comments

Comments

@TrySound
Copy link

TrySound commented Jul 2, 2015

I think this is the most important topic and post it in own issue.
We shouldn't let any plugins have access to another. In this case we need to initialize every plugin call with own instance which will be initialized with some properties of previous instance.

@ghost
Copy link

ghost commented Jul 2, 2015

I see what you mean. How can we achieve this without breaking the current way to compose tasks?

exports.babel = function* () {
  yield this
    .source("babel/src/*.js")
    .babel({ stage: 0 })
    .uglify()
    .concat("all.min.js")
    .target("babel/dist")
}

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran I think we should open 1.0 branch

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran However maybe we can make it without breaks.

@ghost
Copy link

ghost commented Jul 2, 2015

What do you suggest? Basically you don't want other plugins to access other plugins right.

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran FIrstly, plugin should be simpler like

module.exports = function (src, opts) {
  return uglify.minify(src, assign(opts, { fromString: true })).code
};

which will be wrapped with "runner" function

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

Or you have reasons for iterators inside plugins?

@ghost
Copy link

ghost commented Jul 2, 2015

In Fly, a plugin is a the default function exported by a module.

Inside of that function I simply add the method I want to be able to call inside my task to this which is bound to the Fly instance for convenience.

Now, what you are proposing is also interesting. Currently, transform plugins call Fly.prototype.filter(func) to register the filter, but I totally dig your proposal.

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

Or we can make like this. For now at least

module.exports = function (opts) {
  return this
    .filter(function (src) {
      return uglify.minify(src, assign(opts, { fromString: true })).code;
    });
};

@ghost
Copy link

ghost commented Jul 2, 2015

The problem is you want to be able to call this.uglify from inside the task, in your example is impossible to infer this information, unless...

module.exports = function (opts) {
  return this
    .filter("uglify", function (src) {
      return uglify.minify(src, assign(opts, { fromString: true })).code;
    });
};

What do you think? :)

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran The name of the task is module name without fly-*

@ghost ghost added the discuss label Jul 2, 2015
@ghost
Copy link

ghost commented Jul 2, 2015

@TrySound Yeah, that's another option, but having both options would be useful, people will complain that Fly is doing too much otherwise

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran Or if yout want we can try make it like postcss

module.exports = fly.plugin('uglify', function (inst) {
  inst.filter(function (src) {
    return uglify.minify(src, assign(opts, { fromString: true })).code;
  });
});

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran In this case we don't need to wrap inside fly. Just let it user. Also we can validate plugins.

@ghost
Copy link

ghost commented Jul 2, 2015

The problem with this option is that you need to import fly.

Can't we also validate plugins with the previous option anyway?

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

Why not import fly? It still in dependencies.

@ghost
Copy link

ghost commented Jul 2, 2015

It will be if you are using Fly during development, but plugin authors don't need to currently because the Fly instance is bound to this.

Wait, don't you agree that the following is an improvement?

const uglify = require("uglify")
const assign = require("object-assign")

module.exports = function (opts) {
  return this
    .filter("uglify", (src) => 
      uglify.minify(src, assign(opts, { fromString: true })).code)
}

@ghost
Copy link

ghost commented Jul 2, 2015

Either one is fine, but I would prefer the one above because there is only one callback in there, it's a tab block improving the current solution and still looks Fly-ish 😄

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran Okay. I like it too. But still I want to replace this with inst variable. It will be cleaner

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran this will inform plugin developer that he has access to fly instance. But we will give him another object. So... Just semantics.

@ghost
Copy link

ghost commented Jul 2, 2015

@TrySound I wonder how Koa handles this issue, because I was inspired to do this after using Koa in a couple of projects and liking how they handled the use of this inside handlers.

Here is one idea, we can still bind this to an object, that has only the information we want the users / plugin authors to access.

@ghost
Copy link

ghost commented Jul 2, 2015

@TrySound You just said and I didn't notice, 😄 Cool!

Roadmap!

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran So.. function(inst, opts) ?

@ghost
Copy link

ghost commented Jul 2, 2015

...bind this to an object, that has only the information we want the users / plugin authors to access.

Basically what you just said.

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran Not bind, just pass as an argument.

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran Give me access, please. I will open "next" branch.

@ghost
Copy link

ghost commented Jul 2, 2015

@TrySound Do you have a good reason to pass an argument? I see where you are coming from, but one of Fly "features", or design goals from the start was to bind this to a useful context, because it's there and we can use it for free.

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran Okay. Let's use this))

@TrySound
Copy link
Author

TrySound commented Jul 2, 2015

@bucaran Access? Promise will not make direct commits:)

@ghost
Copy link

ghost commented Jul 2, 2015

@TrySound Sure. Hold on.

@ghost
Copy link

ghost commented Jul 2, 2015

@TrySound Welcome aboard 😄

@ghost
Copy link

ghost commented Jul 2, 2015

@TrySound Do you mind coming over to the gitter room?

@ghost ghost closed this as completed Jul 5, 2015
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant