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

about "plugins" and Streams [rambling] #31

Closed
millermedeiros opened this issue Nov 30, 2013 · 13 comments
Closed

about "plugins" and Streams [rambling] #31

millermedeiros opened this issue Nov 30, 2013 · 13 comments

Comments

@millermedeiros
Copy link

Streams are great to deal with large(ish) files and/or when you need to handle too much concurrency (cause memory), but it increases complexity (easier to explain callbacks than streams).

For what I know, most node.js libs are currently built around plain callbacks (fn(data, callback)) and doesn't benefit from Streams (whole data needs to be in memory before processing it).

That got me thinking if writing and encouraging "plugins" is really the best way to go (which I don't think it is). For me it seems more useful to have a helper method that works as an adapter for existing tools than to force everyone to rewrite them as a plugin.

Thinking something like:

// these are regular methods not really meant to be used with Streams, so gulp
// can convert it into a format that works well (no need for custom plugins)
var prependLicense = require('./prepend-license');
var appendToFileName = require('./append-file-name');

gulp.src('src/*.js')
  // `mapFileContent` converts a regular async method into a Stream it passes
  // the second argument to `prependLicense` method (can be any type)
  .pipe( gulp.mapFileContent(prependLicense, {type: 'mit'}) )
  // converts regular method into a gulp file path stream mapper passes "_foo"
  // as second arg to `appendToFileName`
  .pipe( gulp.mapFilePathSync(appendToFileName, '_foo') )
  .pipe( gulp.dest('dist') );

and modules would be just something like:

// append-file-name.js
module.exports = function appendToFileName(path, str, cb){
  return path.replace(/(\.[^\.]+)$/, str + '$1');
};

and:

// prepend-license.js
module.exports = function prependLicense(fileContents, opts, cb){
  fs.readFile('./licenses/'+ opts.type +'.txt', function(err, data){
    if (err) return cb(err);
    cb(null, prepend(fileContents, data));
  });
};

function prepend(src, newData) {
  if (isBuffer(src)) {
    return new Buffer(newData + String(src));
  }
  return newData + src;
}

needs sync and async versions of these methods.

this is somewhat related to #28

@yocontra
Copy link
Member

You can already use event-stream (or other libraries) to wrap existing node-style (callback) libraries. Not sure this belongs in core.

@phated
Copy link
Member

phated commented Nov 30, 2013

I agree that this just looks like event-stream and loses most of what makes
gulp awesome. Gulp can already deal with any type of streams but the
gulp.src & gulp.dest functions return streams that have a custom format
that is specific to gulp.
On Nov 30, 2013 1:24 PM, "Eric Schoffstall" notifications@github.com
wrote:

You can already use event-stream (or other libraries) to wrap existing
node-style (callback) libraries. Not sure this belongs in core.


Reply to this email directly or view it on GitHubhttps://github.com//issues/31#issuecomment-29560231
.

@millermedeiros
Copy link
Author

exactly, the point of these methods would be to convert the custom Stream data format:

{
  shortened: 'foo.md',
  base: 'doc/',
  path: 'doc/foo.md',
  isDirectory: false,
  stat: { ... },
  contents: <Buffer 23 20 63 ...>
}

into something that other tools would normally use (just the path or contents) + some config..

just cause in my opinion it will commit same mistakes as grunt eventually (many useful tools locked to a single runner for no real benefit).

@phated
Copy link
Member

phated commented Nov 30, 2013

Plugins aren't locked in. As long as you pass the expected format stream,
they would work anywhere. Grunt plugins won't work anywhere else. Please,
please, please look at both options more deeply before claiming they have
similar problems.
On Nov 30, 2013 1:40 PM, "Miller Medeiros" notifications@github.com wrote:

exactly, the point of these methods would be to convert the custom Stream
data format:

{
shortened: 'foo.md',
base: 'doc/',
path: 'doc/foo.md',
isDirectory: false,
stat: { ... },
contents: <Buffer 23 20 63 ...>}

into something that other tools would normally use (just the path or
contents) + some config..

just cause in my opinion it will commit same mistakes as grunt eventually
(many useful tools locked to a single runner for no real benefit).


Reply to this email directly or view it on GitHubhttps://github.com//issues/31#issuecomment-29560552
.

@millermedeiros
Copy link
Author

@phated this is just an advice on how you guys could convert other npm modules into a compatible format without having to write custom plugins (and would also simplify the code of current plugins).

@yocontra
Copy link
Member

@millermedeiros I like the idea but I think this belongs in userland. Core is reserved for base functionality not shims for supporting other styles of utilizing the base functionality.

@yocontra
Copy link
Member

Especially if we are thinking of splitting out the file format and standardizing it. The compat layer for that standard shouldn't be in gulp

@millermedeiros
Copy link
Author

just compare the plugin example on the README:

var es = require('event-stream');

module.exports = function(header){
  // check our options
  if (!header) throw new Error("header option missing");

  // our map function
  function modifyContents(file, cb){
    // remember that contents is ALWAYS a buffer
    file.contents = new Buffer(header + String(file.contents));

    // first argument is an error if one exists
    // second argument is the modified file object
    cb(null, file);
  }

  // return a stream
  return es.map(modifyContents);
}

with this implementation:

var gulpMapper = require('gulp-mapper');

module.exports = function(header){
  return gulpMapper.mapFileContentsSync(prepend, header);
};

function prepend(fileContents, str){
  if (!str) throw new Error("header option missing");
  return new Buffer(str + String(fileContents));
}

think of it as a eventStream.map() but for the stream format used by gulp. Could even be a generic method gulpMapper.map() that receives the property name as first argument, like: gulpMapper.mapSync('stat', log) (which would log all the file.stat properties).

@yocontra
Copy link
Member

@millermedeiros Still the same opinion. IMO a function that makes writing plugins/streams easier doesn't need to be in core. It belongs in user-land and if proven useful enough in gulp-util

@millermedeiros
Copy link
Author

@contra sure, it can be on user land as a gulp-mapper module. was more to share my thoughts on what I think could improve the project and reduce the amount of unnecessary plugins (even if it makes the build script a little bit uglier).

@yocontra
Copy link
Member

yocontra commented Dec 1, 2013

@millermedeiros let me know when you have a repo up I'd love to help out

@hparra
Copy link

hparra commented Dec 3, 2013

@millermedeiros I just released a yeoman generator if you haven't started yet:

npm install -g generator-gulp-plugin
mkdir gulp-mapper
cd gulp-mapper
yo gulp-plugin

@yocontra
Copy link
Member

yocontra commented Dec 3, 2013

@hparra Nice! I'll try it out and give some feedback. I can put it in the README next to all of the plugin creation advice

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

4 participants