Add scoped helpers environments from file-utils

[SBoudrias]

Base generator now expose .src and .dest who're file-utils
environment scope as the sourceRoot and destinationRoot. This will allow
access to more file helpers method, and also more transparent process
for filtering, transpiling and validation (conflicter).

This is a follow up commit after #313 discussion. As Grunt team ain't yet moving on exporting Grunt.file into a standalone module, it felt important to me we moved on this one as it felt a major piece for future generator user customization (style guide, JS || coffeescript, etc).

The standalone module is here: https://github.com/SBoudrias/file-utils
And I implemented a simple demo on Generator-bbb: https://github.com/backbone-boilerplate/generator-bbb/compare/remove-grunt (Generator-bbb was using grunt.file so the file methods stay the same and ain't shown clearly in the diff, but the filters are used extensively to write file in the correct style guide).

[addyosmani]

requires a rebase :)

[passy]

Tests don't really work for me. In interactive sessions I get assertion errors because of color codes, non-interactive sessions choke on missing inputs in the conflicter. Could you check if this is happening for you as well, @SBoudrias?

[SBoudrias]

OK, fixed the failing test on Node 0.8 - the build failed because Coveralls seems down.

[jdespatis]

Quite interested in using this PR asap it will be merged ;)

[passy]

Working fine now. Thanks! :)

[SBoudrias]

You think we should release this with 0.14?

[passy]

I'd say so. Can't really break anything and waiting for 0.15 would be annoying.

[SBoudrias]

Yeah, make sense, plus we can consider it as beta feature an improve it from real word usage.

[jdespatis]

And, fmi, have you some release date for the 0.14?

[SBoudrias]

Probably this week.

[jdespatis]

I'm testing this PR, but I'm quite lost...

I have some yeoman prototype function in order to test this.src, like:
(the same way as I would use this.copy())

Generator.prototype.copyFiles = function () {
    this.src.copy("foobar", "foobar2");
};

When I launch the generator, it finds my foobar file I've created in sourceRoot(), but foobar2 is created at the same place as foobar, instead of being in the directory where I am when launching yo

I have the same problem with:

Generator.prototype.copyFiles = function () {
    this.src.copy("foobar", path.join(this.destinationRoot(), "foobar2"));
};

The fact is I have this.destinationRoot() equal to ./
Maybe this is a change of yeoman generator ?

Am I using this.src the wrong way ? What is the correct form ?

Thanks for your help

[SBoudrias]

Can you report what value you get here:

console.log( this.destinationRoot() );
console.log( this.sourceRoot() );

[jdespatis]

@SBoudrias I get (in the same order as you ask for):

./
/path/to/my/generator/app/templates

I've tried to add more log on destinationRoot() and sourceRoot() in the place where you use file.createEnv, and this.sourceRoot() is undefined. However the value is good when I use it

Maybe the problem comes from this.destinationRoot() I would have thought I would get an absolute path

[SBoudrias]

Okay, can you log console.log( process.cwd() );, that should be where ./ resolve as a path.

[jdespatis]

Well the strange thing is that process.cwd is ok,
it returns the current directory where I'm launching yo, and where I expect the foobar2 to be created

[jdespatis]

In order to quickly and dirty overcome this path problem, I use an hardcoded absolute path as destination, and all is ok.
(it works also if I put this.destinationRoot(process.cwd()); before using this.src.copy())
But when I launch my generator twice, in order to check that the file foobar2 is identical, I indeed get a conflict (I should not get a conflict), and when I click on d to see where this diff comes from, yo crashes with the following error:

/path/to/my/generator/node_modules/yeoman-generator-master/node_modules/diff/diff.js:168
    return value.split(/^/m);
                 ^
TypeError: Object foobarcontent
 has no method 'split'

With foobarcontent being the content in my foobar file. The fact is the content is understood as an object and not a String

[SBoudrias]

Is your generator public, I'll fork it and test it later today.

[jdespatis]

I think those problems are global and don't depend on a specific generator

Here is a minimalistic generator that exposes both problem (destinationRoot() problem and conflict found, and crash when showing diff)
(You'll need to create an empty app/templates/foobar)
I've used last yeoman-generator from master branch

var util   = require('util');
var yeoman = require('yeoman-generator');

var Generator = module.exports = function (args, options) {
    yeoman.generators.Base.apply(this, arguments);
};

util.inherits(Generator, yeoman.generators.Base);

Generator.prototype.copyFiles = function () {
    var cb = this.async();

    // Uncomment in order to make the copy work
    // and run again the generator, it will generate a conflict (that should not occur as no file change),
    // that will crash the generator
    //this.destinationRoot(process.cwd());
    this.src.copy("foobar", "other-foobar1");

    this.copy("foobar", "other-foobar2");

    cb();
};

I hope you'll be able to reproduce both problems

[SBoudrias]

@jdespatis Yup, been able to reproduce. I'll send fixes a little bit later.

[SBoudrias]

@jdespatis Bugs fixed:

• Lastest on master, destinationRoot is resolved by default
• 0.1.3 on file-utils, now ensure the file.contents is always a String (previously, you could receive a Buffer object)

[jdespatis]

@SBoudrias feedbacks:

• ok for 0.1.3 on file-utils, it fixes the conflict which was not one, and the crash on diff
• for destinationRoot, yes and no I would say:

-> yes it fixes the issue
-> but the drawback is now, when copying several files, an absolute path is added instead of a relative path for some files (but not all, quite strange). Here is a result when I launch my generator:

identical /path/to/cwd/web/app_dev.php
identical /path/to/cwd/web/config.php
identical /path/to/cwd/web/robots.txt
identical composer.json
identical app/config/routing_dev.yml

I was getting (previously yesterday), which is much cleaner:

identical web/app_dev.php
identical config.php
identical web/robots.txt
identical composer.json
identical app/config/routing_dev.yml

Maybe this strange behavior is a drawback from your fix, or from #401. I do more tests to check

[jdespatis]

Well after a quick test, here are the results:

• the important cosmetic drawback (explained above) is really due to Resolve destinationRoot in prepCopy. #401 (if I comment the line destination = this.isPathAbsolute(destination) ? destination : path.join(this.destinationRoot(), destination);, then I get previous behavior)
• however, even with this change reverted, I get an absolute path when using this.src.copy(). I get this kind of result when launching my generator:

identical /path/to/cwd/myfoobar

I should get:

identical myfoobar

I guess your fix is involved here

Maybe the fix for both would be to just add a cosmetic change when showing identical/create file, so that the path for file is relative to cwd in order to shorten the path as much as possible?

[SBoudrias]

@jdespatis Checkout #409, let me know what you think.

[jdespatis]

I have another question when using file-utils within yeoman-generator.
I suspect a bug, but I may be wrong

I use in fact a filter that beautifies the content of a file, but for some reason, one of my files is invalid and crashes my filter
Therefore, I'd like to catch cleany this exception, but I don't succeed in doing this

As file-utils gives a validationFilter, I've tried to use it, as I thought that the validationFilter was launched before the writeFilter, but it seems not. Is it a bug or the expected behavior ? And in that case, any idea how to catch an exception throwed by the filter ?

Here is a simple generator that shows that the writeFilter is triggerered whatever the validationFilter returns:

var util   = require('util');
var yeoman = require('yeoman-generator');

var Generator = module.exports = function (args, options) {
    yeoman.generators.Base.apply(this, arguments);
};

util.inherits(Generator, yeoman.generators.Base);

Generator.prototype.copyFiles = function () {
    var cb = this.async();

    this.src.registerValidationFilter('normalizer', function (toOutput) {
        console.log("validationFilter has been triggered");
        console.log("===================================");
        console.log(toOutput);
        return 'File is invalid, no write';
    });

    this.src.registerWriteFilter("normalizer", function (file) {
        console.log("writeFilter has been triggered");
        console.log("==============================");
        console.log(file);
        return file;
    });

    this.src.copy("foobar", "other-foobar1");

    cb();
};

I get this on my console:

writeFilter has been triggered
==============================
{ path: '/some/path/other-foobar1',
  contents: 'content\n' }
   create other-foobar1
validationFilter has been triggered
===================================
{ path: '/some/path/other-foobar1',
  contents: 'content\n' }

I keep on getting create other-foobar1 on my console, but this file is never created, because of the validationFilter that returns an error I guess

It would be better I think if the validationFilter could be launched first,
and if it returns true, and only in that case, than the filter could be launched

That way, it would avoid to print create other-foobar1 on the console if the validationFilter forbids to create a file,
And I could have a chance to trap the exception before launching effectively the filter

What do you think of it?

[SBoudrias]

This is expected.

The idea behind write filters is that they modify the file contents or the file name. So, they need to be run before validation in order to validate the actual file to be written.

In your case, the logic should be in the write filter. Can this file be used with the filter? Yes: apply the filter. No: return the file unchanged. You can also use try/catch inside the write filters or anything really.

As a TL;DR

• Write: Modify a file content or a file name (beautification, transpiling js to coffee, etc)
• Validation: Allow a file to be written to disk as is

[jdespatis]

ok, but I still have problem to understand the benefit of validationFilter indeed. Maybe it would be nice to have some examples in the doc ?
The fact is when the validationFilter sends an error, I don't see it, maybe some code is lacking somewhere to handle it ?
and the create other-foobar1 log is shown even if the validationFilter sends an error, and the file is not created in the end, so create other-foobar1 doesn't reflect reality)
Still quite obscure for me ;)

For the exception to be catched, I've added in my writeFilter the following code (self is the Generator), is it the good way ?

this.src.registerWriteFilter("normalizer", function (file) {
    try {
        // beautify file, this operation may throw exception
    } catch (e) {
        var cb = self.async();
        return cb(new Error("Some error"));
    }
    return file;
});

[SBoudrias]

try/catch being synchronous, you're doing a unecessary async handling.

Still, write filters don't expect to be returned an error. If you want to abort when beautification fails, then throw the error... But I think you should work around the error so your generator still produce the expected code (I don't see why beautification failing should break your generator). I'd simply catch the error, and write the file as is without beautification.

The trouble with the logging here, is that all the logging happens in the yeoman conflicter handler (so this is the incorrect behaviour). For the same reason, the error returned by your validation filter is silenced because the conflicter handle the logging. This should be fixed somewhere in the future (there's a lot of work to be done), but it seems to me it is a "minor" bug; I don't see huge use case to custom validation filters on the generator author side.

[jdespatis]

I was doing the var cb = self.async();in order to fetch cb which can show and stop the generator, but I agree it's strange to use it here

In my case, I beautify files, but a file is not a valid json (due to a bug from the generator of sencha grr), and I prefer to break the generator, because this invalid file is really a big error I need to give an attention to

And I'll do an option somewhere to let the filter continue, even for those kind of errors

I've ended with the following strategy:

• If I want the generator to break and show an error, I add in my filter: self.emit('error', new Error("error message")). I've already a code that listens to this signal in my generator
• If I want the generator to only show a log error, but continue the process, then I add in my filter: self.log.error("error message...")

[jdespatis]

Well sorry, but I come back on previous remark.
Ok there's no more conflict when copying a binary file, but the problem now is the fact that the binary content has been changed

If I create a generator in order to create a png file:

var util   = require('util');
var yeoman = require('yeoman-generator');
var Generator = module.exports = function (args, options) {
    yeoman.generators.Base.apply(this, arguments);
};
util.inherits(Generator, yeoman.generators.Base);

Generator.prototype.copyFiles = function () {
    var cb = this.async();
    this.src.copy("foobar.png", "other-foobar1.png");
    cb();
};

You'll see that the 2 png are different, the source is good, but the destination other-foobar1.png is invalid png

[SBoudrias]

Is this your code exactly? (Without filters)

[jdespatis]

The code in my previous post is exactly the code I've used to make the bug appear (png file copied is broken)

You don't succeed in reproducing the problem @SBoudrias ?

[SBoudrias]

Yup, I just pushed a fixed on the lastest master of File-utils. Let me know if it works for you.

Basically, I was pretty wrong in the first fix I made on your comments here. The issue was pretty deep in Grunt.file core and I had to change the default behavior of file.copy to decode text files as Strings.

[jdespatis]

That's strange, I've seen your update, I'm plugged to your master, and on yeoman-generator master, and I still have the problem of png broken

My example of generator previously given is working for you @SBoudrias ?

[SBoudrias]

My example of generator previously given is working for you

No, but it was working after my last update. I didn't publish to NPM though, you'd need to fetch the lastest master to test it. (npm link and etc)

[jdespatis]

Ok sorry, I've had mixed up the file-utils to plug to master
The fact is I use file-utils in my generator also now

As a conclusion, after tests, it works now, png images are all ok
Thanks!

Could you know create a new release and publish it to npm please ?

[jdespatis]

I have another question @SBoudrias
I think this is the expected behavior (in fact hardly sure), but it brings some disturbance indeed for the developer

The fact is, when using file-utils, we get quickly the habitude to use this.src to be relative to this.sourceRoot() and this.dest to be relative to this.destinationRoot()

So when using the handy functions you've put into file-utils, like isDir(), it's weird to get false when we do if (this.src.isDir("my-dir")) with a my-dir that exists into sourceRoot()

It comes from the fact that isDir() is relative to cwd only, and doesn't support the src/dest logic

It's not blocking as this.src.isDir(path.join(this.sourceRoot(), 'my-dir')) will work, but the first 5 minutes are some kind of irrational :)

[SBoudrias]

Yeah, that was the expected behavior, but it may indeed need changes. Also note that you can use file directly:

yo.file.isDir(this.sourceRoot(), 'my-dir');

[jdespatis]

Yes, no disturbance with this format

Well now, I'm happy to wait for the final 0.14 release to base my generator on it :)

[jdespatis]

One more thing @SBoudrias ,
I'm using this.remote() function to fetch a project on github, the remote object returned by this function gives some handy methods like remote.copy

It would be nice there to use file-utils logic

So what would you suggest @SBoudrias ? add file-utils into remote.src, remote.dest for example ?
or to use current available this.src/ this.dest functions with a new sourceRoot() ?

[jdespatis]

@SBoudrias After some tests, adding file-utils feature to the remote doesn't seem to be a big stuff, and would help a lot I think,

The quickly patch I've added into my generator is:

remote.src = self.src;
remote.src._base = remote.cachePath;

remote.dest = self.dest;
remote.dest._destBase = remote.cachePath;

Well, patch is crappy of course as it modifies private vars of file-utils, because not enough API into file-utils

Therefore, why not:
1/ Add a new method file.setEnv() into file-utils that can accept the same options as createEnv, or at least base and dest, in order to be able to modify a file env, already created
2/ then add a PR for yeoman-generator, with some kind of:

remote.src = self.src;
remote.src.setEnv( { base: remote.cachePath } );

remote.dest = self.dest;
remote.dest.setEnv( { dest: remote.cachePath } );

What do you think of it @SBoudrias ?

[jdespatis]

pfiou...
well forget 1/, it's already done

Always look an API twice!, once is not enough...

[jdespatis]

Ant the quick patch is not good, remote.src is not a copy of self.src
I've ended in using createEnv() indeed, and it seems ok

[SBoudrias]

That's the way I'd do it, create a new file.createEnv scoped to the remote location. Mind sending a PR?

[jdespatis]

Yes I'll push one soon

[jdespatis]

@SBoudrias new bug I'm afraid,

The fact is this.src.copy doesn't seem to copy permissions on file.
There was such a bug in yeoman, and has been fixed some time ago

Here is a simple example to reproduce the bug:

var util   = require('util');
var yeoman = require('yeoman-generator');
var Generator = module.exports = function (args, options) {
    yeoman.generators.Base.apply(this, arguments);
};
util.inherits(Generator, yeoman.generators.Base);

Generator.prototype.copyFiles = function () {
    var cb = this.async();
    this.src.copy("foobar.sh", "other-foobar1.sh");
    cb();
};

with chmod +x foobar.sh before launching the generator

You should then notice that other-foobar1.sh hasn't the exec flag

[jdespatis]

Another thing, I'm now intensively using file-utils, all my code has been migrated to it indeed, I no more use this.copy, this.template and so on, and won't use them anymore ;)

And I really think that the handy functions in it like isDir should be scoped also, whenever a relative path is given to it. And if a full path is given, the behavior is the same as before
What do you think of it @SBoudrias ?

[SBoudrias]

That make sense. Would you mind opening issues on file-utils repository so it is easier to track?

[jdespatis]

Right
=> SBoudrias/file-utils#5

[jdespatis]

And
=> SBoudrias/file-utils#6

However, I've seen some commits on file-utils, It seems that some of those issues are already fixed ;)

Ring a bell when you'll publish a new version of file-utils, in order to perform some tests

Thanks for reactivity