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

integrate plugin auto-load in core #141

Closed
michael-ciniawsky opened this issue May 25, 2016 · 17 comments
Closed

integrate plugin auto-load in core #141

michael-ciniawsky opened this issue May 25, 2016 · 17 comments

Comments

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented May 25, 2016

lib/posthtml.js

'use strict';

var parser = require('posthtml-parser');
var render = require('posthtml-render');
// auto-load plugins
var load = require('posthtml-load-plugins')

var api = require('./api.js');

function PostHTML(plugins) {
   // load plugins from package.json with options defined in pkg.posthtml section
   if (plugins === 'undefined') {
     this.plugins = load() || []
   }

   // load plugins form package.json with options defined in external file (e.g plugins.js / plugins.json)
   if (typeof plugins === 'string' )  {
      this.plugins = load(plugins) || []
   }

   // load plugins array as argument
   if (typeof plugins !== 'string') { // or Array.isArray(plugins)
     this.plugins = plugins || []
   }
}

Not tested yet just prototyped here :), if there a no concerns in general i make a PR but gathering feedback first. Plugin order is determined by the position in the options

package.json

"posthtml":  {
  "bem": {}, "↓ bem()"
  "exp": {},    "↓ exp()"
  "add": "plugin", "↓ plugin()"
}

index.js

const posthtml = require('poshtml')

posthtml() // plugins auto-loaded ( e.g [ bem(), exp(), plugin() ] )
  .process(/* file */) 
  .then((result) => result.html)
@awinogradov
Copy link
Member

@michael-ciniawsky I think it's a bad idea to make a special section in package.json. But you can parse deps and find plugins by prefix posthtml- ;)

@michael-ciniawsky
Copy link
Member Author

the section in pkg is one possibility to declare plugin options, plugins are loaded by filtering them with the posthtml- prefix from pkg.deps, pkg.devdeps, you can of course also pass the options as argument to posthtml and only auto-load the bare plugins. the pkg.posthtml section is opt-in, but could be left out completely of course.

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented May 25, 2016

the example above can also be used in this way

plugins.json

{
  "bem": {}, "↓ bem()"
  "exp": {},    "↓ exp()"
  "add": "plugin", "↓ plugin()"
}

index.js

const posthtml = require('poshtml')

posthtml('./plugins.json') // plugins auto-loaded ( e.g [ bem(), exp(), plugin() ] )
  .process(/* file */) 
  .then((result) => result.html)

@awinogradov
Copy link
Member

If you have one more config in project and you will use posthtml in the different ways the idea with pkg.json doesn't work. Also I sure, posthtml-plugins options should be near the posthtml execution, because it's more flexible and readable. You can get inspiration from gulp ;)

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented May 25, 2016

enviroment variables and/or $npm_package_config could be an enhancement.

$npm_package_config
package.json

"config": {
  "posthtml": { 
    "pro": {  "bem": {}, "exp": {}, "minifier": {} },
    "dev": { "bem": {}, "exp": {}, "hint": {} }
  }
}

index.js

const posthtml = require('poshtml')

// this should be handled by posthtml-load-plugins
const options
if (process.NODE_ENV === 'production') {
  options = $npm_package_config_posthtml_pro
}

if (process.NODE_ENV === 'development') {
  options = $npm_package_config_posthtml_dev
}

posthtml(options) // plugins auto-loaded ( e.g [ bem(), exp(), plugin() ] )
  .process(/* file */) 
  .then((result) => result.html)

@awinogradov
Copy link
Member

awinogradov commented May 25, 2016

@michael-ciniawsky it's not about ENV only. In our projects we use posthtml many times with different plugins and all of them depend on ENV. For ex: https://github.com/theprotein/protein-dynamics/blob/master/src/lib/screen.js#L60 and https://github.com/theprotein/protein-dynamics/blob/master/src/lib/screen.js#L78 Why package.json more useful than commonJs module?

@michael-ciniawsky
Copy link
Member Author

With opt-in auto-loading you can reuse your plugin setup in different contexts (grunt, gulp, webpack, middleware etc.) no need to set it up every time. When use posthtml in another context with different requirements you pass the normal plugin array or [plugins-for-task].(js | json) file. Autoloading will only be triggered if no plugins array is given.

@michael-ciniawsky
Copy link
Member Author

It's an additional option it can also stay a seperate plugin if core should not be bloated

@awinogradov
Copy link
Member

Ok ok) I think better way is:

var loadPlugins = require('posthtml-load-plugins');

gulp.task('html', function() {
    let posthtml = require('gulp-posthtml');
    return gulp.src('src/**/*.html')
        .pipe(posthtml( /* auto load them all */ loadPlugins() /*, options */))
        .pipe(gulp.dest('build/'));
});

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented May 25, 2016

yes that's how posthtml-load-plugins currently works, despite the options are an argument to posthtml-load-plugins itself.

var loadPlugins = require('posthtml-load-plugins');

gulp.task('html', function() {
    let posthtml = require('gulp-posthtml');
    return gulp.src('src/**/*.html')
        .pipe(posthtml( /* auto load them all */ loadPlugins(options))
        .pipe(gulp.dest('build/'));
});

with load-plugins in core

var posthtml = require('gulp-posthtml');

var options = {}
var options2 = require('./plugins.json')
var options3 = require('./plugins2.json')

gulp.task('html', function() {
    return gulp.src('src/**/*.html')
        .pipe(posthtml( /* auto load them all, */ options)
        .pipe(gulp.dest('build/'));
});

gulp.task('html2', function() {
    return gulp.src('src/**/*.html')
        .pipe(posthtml( /* auto load them all, */ options2)
        .pipe(gulp.dest('build/'));
});

gulp.task('html3', function() {
    return gulp.src('src/**/*.html')
        .pipe(posthtml( /* auto load them all, */ options3)
        .pipe(gulp.dest('build/'));
});

gulp.task('html-pkg', function() {
    return gulp.src('src/**/*.html')
        .pipe(posthtml( /* auto load them all, */))
        .pipe(gulp.dest('build/'));
});

gulp.task('html-inline', function() {
    let plugins = [];
    return gulp.src('src/**/*.html')
        .pipe(posthtml(plugins))
        .pipe(gulp.dest('build/'));
});

@awinogradov
Copy link
Member

I don't see enhancements, really ;( @zxqfox maybe you?

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented May 25, 2016

the enhancement is that you have a single 'source of truth' for your common plugin config and no declaration of the plugins in your webpack.config.js, gulpfile.js, Gruntfile.js, no posthtml-cli --config xyz.json etc. the options define which plugins are loaded from pkg.json if you have 20 plugins as deps only the one given options for the specific tasks are loaded.e.g options1 has options for 2 plugins => these two are loaded with there options in order od declaration => [plugin(options [0], plugin1(options[1])] and so on.

pkg.json

"dependencies": {
   "posthtml-bem": "*",
   "posthtml-exp": "*",
   "posthtml-import": "*"
},
"devDependencies": {
  "posthtml-hint": "*"
}

plugins.json

{
  "import": {"root": "./html/" },
  "bem": { "elemDltr": "__",  "modDltr": "--" },
  "exp": { "locals": { "hello": "world"} }
}

posthtml-load-plugins as separted plugin

const posthtml = require('posthtml')
const options = require('./plugins.json')

const loadPlugins = require('posthtml-load-plugins')(options)

// in order with options applied, posthtml-hint not required
// this.plugins = [import(plugins.import), bem(plugins.bem), exp(plugins.exp)]
posthtml(loadPlugins).process...

posthtml-load-plugins as part of posthtml

const posthtml = require('posthtml')
const options = require('./plugins.json')

// in order with options applied, posthtml-hint not required
// this.plugins = [import(plugins.import), bem(plugins.bem), exp(plugins.exp)]
posthtml(options).process...

All i ask if there is interest in making this a core functionality, being a separate module like posthtml-render / posthtml-parser, so i can remove it form posthtml-loader and posthtml-cli etc. or it should better stay as a plugin ;)

@qfox
Copy link
Collaborator

qfox commented May 25, 2016

I feel like you trying to bring into the core functionality you love but it looks like a separate thing. If we will go this way we should support configuration, loaders, etc. but what is the purpose? For now posthtml core is just composition of html parser and runner for plugins-transformers. Personally I prefer to keep it simple.

The only thing I want to note that we can slightly modify core's API to simplify loader's API and to make it more user friendly. Suggestions are welcome!

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented May 25, 2016

I feel like you trying to bring into the core functionality you love but it looks like a separate thing.

@zxqfox Well, yes in some ways, of course i'm trying :D, but i don't want people to e.g write all declarations in uppercase letters or with a max of 3 characters and then suggest everyone should be forced to do it the same way.I take responsibility and it's by no intend of mine a pure egoistical/unreflected request.

For now posthtml core is just composition of html parser and runner for plugins-transformers.

imho plugins-transformers === plugins to load => load of plugins is a core requirement besides parsing and rendering html (processing).

If we will go this way we should support configuration, loaders, etc. but what is the purpose?

Loader (posthtml-loader), CLI (posthtml-cli) currently are shipping with posthtml and posthtml-load-plugins as dependencies and the logic for loading plugins is implemented there, this could also be done by the posthtml function itself, load-plugins is a API not to simplify the loader's API but to make it more user friendly. It's completely optional and backwards compatible. It just ships with posthtml as a dependency which is required and only required, when you provide the rigth input, otherwise it's used as before. But in posthtml.js the PostHTML function needs to check the input and then require load-plugins.

Despite the input checking in the constructor function core stays completely untouched.

node_modules
|
|-posthtml
|      |-posthtml-parser
|      |-posthtml-render
|      |-posthtml-load-plugins

lib/posthtml.js

'use strict';

var parser = require('posthtml-parser');
var render = require('posthtml-render');

var api = require('./api.js');

function PostHTML(plugins) {
   // plugins file as argument (e.g plugins.js / plugins.json)
   if (typeof plugins === 'string' )  {
      this.plugins = require('posthtml-load-plugins')(plugins) || []
   }

   // plugins array as argument
   if (typeof plugins !== 'string' || Array.isArray(plugins)) {
     this.plugins = plugins || []
   }
}
const posthtml = require('posthtml')

// 1. auto loading
posthtml('./plugins.json').process...

// 2. manual loading
const plugin1 = require('plugin1')(options)
const plugin2 = require('plugin2')(options)

posthtml([plugin1(), plugin2()]).process...

use either, depending on your preference and what's appropiated :)

All third-party wrappers then can use both by default(cli, grunt, gulp, broccoli, webpack, express, hapi, koa, metalsmith)

But if you reject just say it then it will be dealt in the different modules

@qfox
Copy link
Collaborator

qfox commented May 25, 2016

imho plugins-transformers === plugins to load => load of plugins is a core requirement besides parsing and rendering html (processing).

It's a juggling because loading files is not required if you passing js functions (most of cases). You need to load plugins with configs only when you prefer to save configurations separately. That's what posthtml-load-plugins do, right?

// 1. auto loading
posthtml('./plugins.json').process...

If I got it right, we can go this way and so on to keep core simple.

posthtml(require('posthtml-configurate')('./plugins.json')).process…
// or
posthtml(require('./plugins.js')).process…

But if you reject just say it then it will be dealt in the different modules

I'm afraid you got my answer incorrectly because I can't reject anything, it's an open source project without any kind of dictate (at least I hope in it). But I'm trying to bring some facts to show you why I don't like this idea and think that it won't help much. I could be wrong but downloading stats shows that posthtml-load-plugin is used by 10% of all posthtml users.

@awinogradov
Copy link
Member

@michael-ciniawsky now you can store options in package.json and require them. We should do nothing to support this. Auto-loader it's super helper, but no one plugin shouldn't change the core. It's the main idea. The core is simple and does nothing in general. If we know the way to avoid core changing we should choose this way. And now I'm sure It is that case.

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented May 25, 2016

I'm afraid you got my answer incorrectly because I can't reject anything, it's an open source project without any kind of dictate (at least I hope in it).

@zxqfox sure, it's not meant in a bad mood :)

If I got it right, we can go this way and so on to keep core simple.

posthtml(require('posthtml-configurate')('./plugins.json')).process…
=== // i assume you meant it that way :)?
posthtml(require('posthtml-load-plugins')('./plugins.json')).process...
// or
posthtml(require('./plugins.js')).process…

forget about the separate file for a moment, it would be also possible doing this.

const posthtml = require('posthtml')

// posthtml-bem, posthtml-exp in pkg.json
const plugins = {  //order
  bem: {},   //1.
  exp: {},   //2.
}

posthtml(require('posthtml-load-plugins')(plugins)).process...
// plugins = [bem({}), exp({})]

// or

// obj directly gets passed to loader API
posthtml(plugins).process…
// plugins = [bem({}), exp({})]

load-plugins takes also care of requiring the plugins (lookup in pkg.json) and scaffolding the plugins array, you can declare options but it's no requirement. One benefit is that you don't need to require the plugins manually in the file anymore (var plugin = require('....')etc...) just the name without the posthtml- prefix and with/without options declared as values. The obj[i].key then determines at which index the loader API pushes the plugins to the plugins arrray.

I could be wrong but downloading stats shows that posthtml-load-plugin is used by 10% of all posthtml users.

It's an unoffical way from an average joe (me) which needs extra reading and an extra step at the moment, the downloads stats for that should be 0, 0%, nada... at best :). it's not that it must be exactly that plugin/module it could be separate file in posthtml or a function in api.js.

but kk leave it as a plugin for the time being.

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

3 participants