dbashford / mimosa

A lightning-fast, modular, next generation browser development tool.
http://mimosa.io/
519 stars 34 forks source link

Possible node / file.coffee error with binding.stat on windows? #181

Closed alextreppass closed 11 years ago

alextreppass commented 11 years ago

Hi,

I'm running Node 0.10.5, Mimosa 0.11.12.

We've got a custom mimosa module which watches certain folders for images, copies those into an 'icons' folder in the compiledDir and then writes a simple json file with some metadata about those icons.

We've registered the 'write json to file' method in the 'postBuilt', 'init' step.

The method is roughly:


if (!fs.existsSync(path.dirname(outFile))) {
        logger.info("mimosa-icon-engine[INFO]: Directory does not exist for file " + outFile  + ", creating now.");
        wrench.mkdirSyncRecursive(path.dirname(outFile));
    }
    var unescapeData = data.replace(/\\\\/g, '/');
    var outData = "define(" + unescapeData + ");";
    logger.info('mimosa-icon-engine[INFO]: _writeJsonToFile about to writefileSync: ' + outFile);
    fs.writeFileSync(outFile, outData, 'utf8');
    logger.success("mimosa-icon-engine[LOG]: Combined json object successfully written to " + outFile);

This works for my colleagues who are on node 0.99 and mimosa 0.10.3, but not for me on the latest version of node and mimosa.

In the console just before it bombs I get:

10:26:25 - mimosa-icon-engine-mapping [INFO]: writing...
10:26:25 - ICON MAPPING ::: { /* icon filenames to metadata */ }
10:26:25 - mimosa-icon-engine-mapping [INFO]: _writeJsonToFile: /* path to icons-mappi
ng-json.js */

fs.js:166
  binding.stat(pathModule._makeLong(path), cb);
          ^
TypeError: path must be a string
    at Object.fs.exists (fs.js:166:11)
    at D:\Users\ATreppass\AppData\Roaming\npm\node_modules\mimosa\lib\util\file.
coffee:82:19
    at Object.cb [as oncomplete] (fs.js:168:19)

And it never prints out the success part.

The line position 82:19 is seemingly for the file.coffee compiled down to js, as 82 in the coffee script is blank.

I don't know if this is an issue with the writeFileSync call we're doing in our module, or whether it's a callback from somewhere else on the event queue that happens to be bombing. It's almost certainly our module.

Running mimosa build --debug doesn't shed any light.

If I roll back to Node 0.9.9 (keeping mimosa at latest) then things work fine.

Any ideas?

dbashford commented 11 years ago

That is a pretty basic node 8 to 10 error. node 10 made changes such that all file paths need to be strings. The big hit on that is usually path.join.

So you are starting up another watcher for this, or using the existing watching? If you are writing modules that fire up more watchers on top of the existing ones, it may explain some of the other issues you've been having with CPU. There might be a good way to leverage the existing watcher/registration to do this for you.

Do me a favor, between these two lines: https://github.com/dbashford/mimosa/blob/master/lib/util/file.coffee#L44-L45 could you console log the two file names and let me know what they are? One is likely null or undefined.

dbashford commented 11 years ago

Try out the just released v0.12.0. I added a few lines that might take care of this specific issue. Bit of a shot in the dark, but I have hope.

It also may just kick the can down the lifecycle to something else that'll have trouble with null/undef file paths.

alextreppass commented 11 years ago

That actually fixed things! Very nice stab in the dark :) What was the issue in the end?

Regarding the watchers, we spawn a new watcher for each folder specified in our 'icon folders' config. (It turns out we're only using one path in there at the moment, which happens to be 'css' i.e. already watched by mimosa).

Is there a way to interact with the existing watcher, to be able to specify additional file paths (relative to assets dir or absolute) that should be watched, and it can silently skip folders that it's already watching?

Cheers

dbashford commented 11 years ago

Well, the error you posted could have only been occurring in one place in that code. And its indicative of some non-string (likely null or undefined) making its way into the function, so I just check for those. Its not a bad check to be making, just to be on the safe side of things, but I would love to know how it was happening in the first place. I'm sure there's some valid reasons for it happening and with node10 it just matters now, since node gripes about passing non-string values into a lot of its fs and path apis.

If you need to watch directories outside of the watch.sourceDir, then yeah, you need your own watcher. There is certainly plenty of precedent for it. import-source, live-reload, and server-reload all need their own watchers. I'd just make sure you are scoping the size of the watch down to just the things you want to watch. Not sure what watcher you are using, but if it is chokidar (what mimosa uses), there's no need to spawn new watchers, chokidar can take an array of paths.

I've wanted to be able to add additional folders to the watching for awhile, but it would be a pretty big refactor.

You can skip folders you are currently watching using the watch.exclude.

dbashford commented 11 years ago

Welcome to keep up the discussion, but going to close this out.