Yeoman Generetor Crashes Because of endless recursion

[korczis]

Hi There,
I think latest version of yeoman generator has some bug which crashes it when some amout of files is exceeded. Me and one other githuber thought it is chalk.js issue, but we were wrong. Author of chalk did some investigation and pointed to generator.

Can you take a look please?

Origin (chalk.js) issues: chalk/chalk#4

The simpler test case I can think of is:

Run yo generator (let's say the generator is called "test")
Replace all prototype functions in app/index.js with:

TestGenerator.prototype.getWp = function getWp() {
  var done = this.async();

    this.log.writeln('Getting WordPress v3.6');

    this.remote('wordpress', 'wordpress', '3.6', function (err, remote) {
        if (err) {
            done(err);
        }
        remote.directory('.', '.');
        done();
    });
};

Run yo test

[Volox]

I don't know if it solves your problem, but before each done call you should really put a return statement. Otherwise in case of error the done method will be called twice and the remote.directory('.', '.'); will be executed regardless.

[korczis]

Hi, I would like to try this, but my generator looks bit different and I am not sure where should I put it in my case.

Please, can you take a look at https://github.com/korczis/generator-the-scratch/blob/master/app/index.js and help me investigate if can I fix it on my own?

[jahvi]

@Volox I was the one that originally opened the issue on the chalk repo, I tried your suggestion but no luck still got the same error

[korczis]

Any news?

[jahvi]

I wonder if this is a Windows issue, @korczis what OS are you using?

[korczis]

It crashes on both - Linux Mint 64 bit in Virtual Box and Windows 7 64 Bit Ultimate.

[korczis]

Btw, my generator is here: https://github.com/korczis/generator-the-scratch

# Clone repo
git clone https://github.com/korczis/generator-the-scratch.git

# Init submodules
git submodule init

# Update submodules
git submodule update

# install modules
npm install

# link the generator
npm link

And now you can run the generator

yo the-scratch

[aaronbushnell]

I'm also getting the following error when trying to copy over files and folders from a large directory within my generator. The first couple hundred copy over fine, but eventually breaks and includes this message:

/usr/local/lib/node_modules/yo/node_modules/chalk/chalk.js:30
                var obj = defineProps(function self(str) {
                          ^
RangeError: Maximum call stack size exceeded

[jahvi]

Yep same error, I get it after ~486 files

On Wed, Sep 11, 2013 at 9:04 AM, Aaron Bushnell notifications@github.comwrote:

I'm also getting the following error when trying to copy over files and
folders from a large directory within my generator. The first couple
hundred copy over fine, but eventually breaks and includes this message:

/usr/local/lib/node_modules/yo/node_modules/chalk/chalk.js:30
var obj = defineProps(function self(str) {
^
RangeError: Maximum call stack size exceeded

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/350#issuecomment-24239878
.

Ing. Javier Villanueva

[wesleytodd]

I am having the same error, and was able to run the generator without this problem at 0.11.4 but not at 0.12.0. I will look into it a bit more by examining the changes between those releases and see if I can find anything.

[wesleytodd]

For anyone else who is interested: v0.11.4...v0.12.0

[wesleytodd]

2 hours and I managed to dig into some async.js code, maybe someone who has more familiarity with this code can figure it out. Currently I am going to keep my generator at v0.11.4 and hope someone else might have more luck.

[jahvi]

Thaks for pointing the version out, I'll stick with 0.11.4 as well

On Sat, Sep 14, 2013 at 3:24 PM, Wes Todd notifications@github.com wrote:

2 hours and I managed to dig into some async.js code, maybe someone who
has more familiarity with this code can figure it out. Currently I am going
to keep my generator at v0.11.4 and hope someone else might have more
luck.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/350#issuecomment-24457458
.

Ing. Javier Villanueva

[aaronbushnell]

Hm, I'm changed my package.json file to

"dependencies": {
  "yeoman-generator": "~0.11.4"
},

but I'm still getting this error when running my yo command:

/usr/local/lib/node_modules/yo/node_modules/chalk/chalk.js:30
                var obj = defineProps(function self(str) {
                          ^
RangeError: Maximum call stack size exceeded

[jahvi]

You probably need to reinstall the package it might still be referencing
the previous version

On Sat, Sep 14, 2013 at 11:08 PM, Aaron Bushnell
notifications@github.comwrote:

Hm, I'm changed my package.json file to

"dependencies": {
"yeoman-generator": "~0.11.4"
},

but I'm still getting this error when running my yo command:

/usr/local/lib/node_modules/yo/node_modules/chalk/chalk.js:30
var obj = defineProps(function self(str) {
^
RangeError: Maximum call stack size exceeded

—

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

Ing. Javier Villanueva

[wesleytodd]

You need to get rid of the tilde. That says to install a compatible version. What you want is a specific version.

[jahvi]

That shouldn't matter, as I understand the tilde means to match v0.11.* so it'll fetch the 0.11.5 version (not sure if that one has the bug though)

[aaronbushnell]

Using "yeoman-generator": "0.11.4" must not be a complete fix of this issue. I tried to apply that same dependency rule to @addyosmani's generator-boilerplate and it gave me a RangeError: Maximum call stack size exceeded error.

[SBoudrias]

I've been able to reproduce replicating steps linked by @korczis here: #350 (comment) (on OSx)

[SBoudrias]

@sindresorhus I wonder if the solution on Yeoman side is to use Streams instead of sync read/write on each repo file. Streams auto regulate the amount of data they ingurgitate to prevent overflows, but I don't think this would really slowdown the speed of the logs...

[addyosmani]

If streams would provide a more stable experience, why not?

[sindresorhus]

@SBoudrias i agree

[wesleytodd]

Streams would probably reduce the peak memory usage, but I am not sure that would fix this issue. And I am not sure how streaming template parsing works with underscore, so it might be difficult.

[SBoudrias]

@wesleytodd Yeah, that was my second though too.

Although, I don't think large/bulk copying need processing. So maybe it'd be better to have two kind of copying, "on by one" (filter, processing) and "bulk" (just copy a full dir with 3000 files).

(I checked around Grunt yesterday to see if they ever had similar issues, at it don't look like it)

[wesleytodd]

I found the fix. I am putting together the PR right now. It was the async.series call.

[wesleytodd]

Looks like it failed the tests, I probably should have ran them before submitting the PR, sorry. Let me see about that and I will update with whatever changes are necessary.

[wesleytodd]

@SBoudrias I like the idea of doing a "bulk" copy operation that does not check for conflicts or parse templates. I have a rough implementation of this by just adding a fourth parameter to the copy and directory methods which skips the conflicter and engine steps.

Would you guys rather have this api change, or a completely new method like bulkDirectory() or bulkCopy()?

[korczis]

I would vote for second option.

One more point. Will not be also usefull have some white/black listing variant? Imagine the situation where one have large folder structure with tens of dirs and subdirs and wants to have only few of them processed by conflicter and engine.

It will be nice to have posibility to pass input dir and list of files to have processed by engine.

What do you think of it?

[wesleytodd]

@korczis Sorry, I was already in it on this method when I read your post. Maybe if anyone on the core team could chime in a say what they would like. It would only take a few minutes to port my PR over to a separate method instead of an extra parameter.

[Maricek]

Also experiencing this ...

[aaronbushnell]

@wesleytodd Thanks for making that commit! How can I implement this into my Yeoman generator? Got a bit of documentation on it?

[wesleytodd]

@aaronbushnell Check out #359. You could apply that patch on your version of yeoman-generator. But that is probably not recommended. I would either revert back to v0.11.4 for your project or just wait until that PR gets resolved and released to a new version of yeoman.

[SBoudrias]

Should be fixed with #359!

[jahvi]

I just updated the package but it looks like the directory method isn't running the new bulk copy function since I'm getting the same error, I changed my code to:

WpGenerator.prototype.getWordpress = function getWordpress() {
    var done = this.async();

    this.log.writeln('Getting WordPress v' + this.wpVersion);

    this.remote('wordpress', 'wordpress', this.wpVersion, function (err, remote) {
        if (err) {
            done(err);
        }
        remote.directory('.', '.', false, true); // <-- Is it ok?
        done();
    });
};

Maybe I'm doing something wrong.

[SBoudrias]

You need to use the new methods, bulkDirectory and bulkCopy.

.directory() behavior is still unchanged.

[jahvi]

Strange I'm getting a "TypeError: Object # has no method 'bulkDirectory'" error when I change it from directory to bulkDirectory even though the function is there on the actions.js file.

[korczis]

Folks, may I ask you what the situation is now?

[wesleytodd]

@korczis There are two new methods from this PR: #359

The methods are bulkCopy and bulkDirectory. These skip the normal template processing and do not do the conflicter check for each file. Here are links to the new methods:

https://github.com/yeoman/generator/blob/master/lib/actions/actions.js#L175
https://github.com/yeoman/generator/blob/master/lib/actions/actions.js#L387

You use these just like you used to with the normal copy and directory. Make sense?

[jahvi]

I'm trying to use the bulkDirectory method, however it gives me the error
I copied above saying the function couldn't be found. I updated the package
with the latest master branch and I checked the files and it looks like the
function is there which is weird.
On Oct 20, 2013 1:40 PM, "Wes Todd" notifications@github.com wrote:

@korczis https://github.com/korczis There are two new methods from this
PR: #359 #359

The methods are bulkCopy and bulkDirectory. These skip the normal
template processing and do not do the conflicter check for each file. There
are links to the new methods:

https://github.com/yeoman/generator/blob/master/lib/actions/actions.js#L175
https://github.com/yeoman/generator/blob/master/lib/actions/actions.js#L387

You use these just like you used to with the normal copy and directory.
Make sense?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/350#issuecomment-26678721
.

[wesleytodd]

@jahvi Ha, good catch. The methods were not added for the remote usage. I will add those and submit another PR.

[c00kiemon5ter]

Hi all, I've fall for this too :(

While the new bulk functions seem to address the stack limits exhaustion for copy and directory I am getting this on a template call too.

identical foo/bar/baz1.html
identical foo/bar/baz2.html
[...]
identical foo/bar/baz50.html

/usr/lib/node_modules/yo/node_modules/chalk/chalk.js:30
                                var obj = defineProps(function self(str) {
                                          ^
RangeError: Maximum call stack size exceeded

and as template write over the rendered file, this could be related.

[jahvi]

@wesleytodd I switched to your remote-bulk-copy branch and it works fine, however just before the copy I get the following wraning:

conflict .
[?] Overwrite .? (Ynaxdh)

Even though the target directory is empty, selecting "yes" copies the files just fine but I'm not sure why the conflict.

[wesleytodd]

@jahvi The new bulkDirectory method only checks one time for the existence of the directory, rather than checking for every file. I added this by recommendation @SBoudrias and I quite agree that it should be there. If you know the directory is empty then you can just Y and move along. Otherwise people might accidentally overwrite directories, Yikes! :)

Is there a better way that you can think of to handle this?

[SBoudrias]

I think the issue here is that the . path ain't clear. Maybe you could path.resolve() what is shown there... But I feel like this is a bit of a edge case. I don't feel like any generator should dumb a bunch of code to the root...

[wesleytodd]

@SBoudrias It would be pretty trivial to get the dir name when it is just a .. I can throw that in later today if you guys like that idea.

[SBoudrias]

Yup, but I would like to reduce complexity to a minimum. And I don't think we should resolve every path as relative path are usually great for the display purpose.

You can send a PR and we'll see if it is simple enough 👍

[getdave]

For anyone who is interested I ended up working around by using https://npmjs.org/package/ncp

  this.remote('wordpress', 'wordpress', '3.6', function (err, remote) {
    if (err) {
        cb(err);
    }

    ncp(remote.cachePath, path.join(site_url, 'wp'), function (err) {
      if (err) {
        return console.error(err);
      }
      cb();
    });  
  });

Obviously site_url is a global var defined earlier in my generator.

[SBoudrias]

@getdave Awesome module, thanks for coming with it.

[korczis]

@wesleytodd Perfect. Thanks. 👍