FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixed #899 - browserify removes output file if it didn't exist previously by kika · Pull Request #1239 · browserify/browserify · GitHub

Repository navigation

Fixed #899 - browserify removes output file if it didn't exist previously - #1239

Closed
kika wants to merge 3 commits into
browserify:masterfrom
rackmaze:master
Closed

kika wants to merge 3 commits into
browserify:masterfrom
rackmaze:master

Conversation

kika commented May 2, 2015

Copy link
Copy Markdown

The change is trivial. I didn't use fs.exists() because it's being deprecated soon according to the recent Node documentation.

zertosh commented May 6, 2015

Copy link
Copy Markdown
Member

Why the tmp stream? Instead of:

try {  fs.unlinkSync(outfile); } catch(err) { }

kika commented May 6, 2015

Copy link
Copy Markdown
Author

Generic answer : I personally hate software which does what it wasn't asked to do :-)
Specific answer: if, for whatever reason, the output file did exist before the browserify run it will get deleted without advance warning. So I check if the file exists before attempt to run and clean up after the failed run only if there was no output file before.

zertosh commented May 6, 2015

Copy link
Copy Markdown
Member

Ah makes sense. The fs.createWriteStream(outfile) will write to the file (thus emptying it if it existed before) pretty much always before any error ever gets emitted by browserify. But at least you're leaving the file behind. Ok cool, but that tmp stream 🙀

var outfileExists;

if (outfile) {
    try { outfileExists = !!fs.lstatSync(outfile); } catch (err) {}
    bundle.pipe(fs.createWriteStream(outfile));
}

kika commented May 6, 2015

Copy link
Copy Markdown
Author

makes sense, thanks. Updated.

ghost commented May 6, 2015

Copy link
Copy Markdown

I'm against adding .sync calls. This patch also seems to add a lot more complexity that will probably break in strange ways. Writing to a tmp file was removed from watchify for this reason since it kept breaking on windows. I'm skeptical that this kind of patch won't ripple up to create problems.

kika commented May 6, 2015

Copy link
Copy Markdown
Author

@substack, are you against this PR entirely, or just against replacing the test for existence by opening the stream with lstatSync()? Without this PR (or equivalent) browserify breaks make-based workflows and forces use of make kludges like DELETE_ON_ERROR which do not exist in all flavors of make

Copy link
Copy Markdown
Member

It looks like #899 was addressed by #1673. Thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL