browserify / watchify

watch mode for browserify builds
Other
1.79k stars 181 forks source link

Always wait for dotfile stream before renaming #169

Closed zertosh closed 9 years ago

zertosh commented 9 years ago

Should fix the infamous Error: EPERM, rename error. If this works, I'll add another commit writing the temporary dotfile in the tmp directory instead of in the output file's directory. I'm not doing that now because I want to isolate the true fix of the EPERM error.

zertosh commented 9 years ago

UGH failed tests... old steams -_-

ArtskydJ commented 9 years ago

@zertosh I tested this PR, and it works great!

Except for one little thing. After an error logs, it also logs out an incorrect number of bytes written to the file. (In verbose mode, -v.)

C:\Users\Michael\Github\javascript\state-router-example>watchify implementations\virtualdom\index.js -d -v -o implementations\virtualdom\bundle.js
473839 bytes written to implementations\virtualdom\bundle.js (2.04 seconds)
473839 bytes written to implementations\virtualdom\bundle.js (0.45 seconds)
Error: Parsing file C:\Users\Michael\Github\javascript\state-router-example\implementations\virtualdom\index.js: Unexpected token (8:33)
473839 bytes written to implementations\virtualdom\bundle.js (0.45 seconds)

At this point, bundle.js is:

console.error("Error: Parsing file C:\\Users\\Michael\\Github\\javascript\\state-router-example\\implementations\\virtualdom\\index.js: Unexpected token (8:33)");
zertosh commented 9 years ago

@ArtskydJ Oh yeah it's using the old values of bytes and time - I'll fix that. I'm stuck trying to figure out how old node 0.8.x streams work since that's causing the tests to fail.

zertosh commented 9 years ago

Not "old" - more like the last "success" values

ArtskydJ commented 9 years ago

@zertosh It used to only log the error when a error occurred. Perhaps this could be implemented by doing bytes = 0; here, and doing if (verbose && bytes) { here.

Unfortunately it's slightly hacky.

ArtskydJ commented 9 years ago

A lot of people support the current and the previous stable version of node. Right now, that's 0.12 and 0.10, so maybe 0.8 could be dropped? This is a matter of opinion. (I have no idea how many people still use 0.8.)

zertosh commented 9 years ago

@ArtskydJ I have no idea either - but I figured the problem so I'll just let it be. For the bytes count I was thinking of:

        var body = 'console.error('+JSON.stringify(String(err))+');';
        bytes = body.length;
        time = 0;
        dotStream.end(body);

but that's so ugly :cry:

zertosh commented 9 years ago

Green build :smile:

ArtskydJ commented 9 years ago

If a watchify build errors, I don't really care how long it took, or how many bytes were written.

But maybe that's just me. :smiley:

zertosh commented 9 years ago

lol fair enough - but you gotta keep it consistent. I just ran a quick test in node 0.12.1, and the tests hang randomly - but when I bump chokidar to 1.0.0-rc4, they run just fine. I'll add that version and maybe iojs to the travis.yml in 3.0.0.

zertosh commented 9 years ago

@ArtskydJ once I get a few more confirmations that this fix works I'll do a release - so hopefully tmw

ArtskydJ commented 9 years ago

@zertosh I don't know if it matters, but the log output changes with this PR.

It used to log either an error, or this message: "x bytes written to y.js (z seconds)"

Now if there is an error, it still logs the message.

What I get:

watchify files\main.js -v -o bundle.js
788 bytes written to bundle.js (0.10 seconds)
Error: Parsing file C:\Users\Michael\Github\javascript\watchify\example\files\main.js: Unexpected token (2:18)
136 bytes written to bundle.js (0.00 seconds)

What I expect:

watchify files\main.js -v -o bundle.js
788 bytes written to bundle.js (0.10 seconds)
Error: Parsing file C:\Users\Michael\Github\javascript\watchify\example\files\main.js: Unexpected token (2:18)

Other than that, everything works great! Again, I don't know if it matters. I just thought I'd let you know.

Thanks for figuring this out!

zertosh commented 9 years ago

@ArtskydJ You're right, the message and the error were mutually exclusive. I'll fix that and do the release.

ArtskydJ commented 9 years ago

@zertosh Awesome!

zertosh commented 9 years ago

Published as watchify@2.6.2

manuelcabral commented 9 years ago

Seems to be working fine here.