Skip to content

Always wait for dotfile stream before renaming#169

Merged
zertosh merged 1 commit intomasterfrom
fix-windows-eperm
Mar 26, 2015
Merged

Always wait for dotfile stream before renaming#169
zertosh merged 1 commit intomasterfrom
fix-windows-eperm

Conversation

@zertosh
Copy link
Copy Markdown
Member

@zertosh zertosh commented Mar 26, 2015

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
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

UGH failed tests... old steams -_-

@ArtskydJ
Copy link
Copy Markdown

@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
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

@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
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

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

@ArtskydJ
Copy link
Copy Markdown

@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
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

@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 😢

@zertosh
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

Green build 😄

@ArtskydJ
Copy link
Copy Markdown

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. 😃

@zertosh
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

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
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

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

@ArtskydJ
Copy link
Copy Markdown

@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
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

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

@ArtskydJ
Copy link
Copy Markdown

@zertosh Awesome!

@zertosh zertosh force-pushed the fix-windows-eperm branch from 5a430f0 to a0b1c0f Compare March 26, 2015 14:39
zertosh added a commit that referenced this pull request Mar 26, 2015
Always wait for dotfile stream before renaming
@zertosh zertosh merged commit 4365fa4 into master Mar 26, 2015
@zertosh zertosh deleted the fix-windows-eperm branch March 26, 2015 14:59
@zertosh
Copy link
Copy Markdown
Member Author

zertosh commented Mar 26, 2015

Published as watchify@2.6.2

@zertosh zertosh mentioned this pull request Mar 26, 2015
7 tasks
@manuelcabral
Copy link
Copy Markdown

Seems to be working fine here.

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

Successfully merging this pull request may close these issues.

3 participants