Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "fs: Revert throw on invalid callbacks" #18668

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 9, 2018

This reverts commit 8250bfd.

Quite some time has passed since the deprecation was originally added. So now multiple major versions contained the deprecation notice and people have hopefully moved on.

I would really like to get this in v.10. It was originally planned to get this in earlier again.

Refs: #12562
Refs: #12976

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 9, 2018
@BridgeAR BridgeAR requested a review from a team February 9, 2018 00:47
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 9, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2018

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


Calling an asynchronous function without a callback is deprecated.
Calling an asynchronous function without a callback will throw a `TypeError`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be present tense: "... without a callback throws a TypeError from Node XX onwards."

lib/fs.js Outdated

mode = mode | 0;
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong change. makeCallback() returns a function that runs callback in the global scope, i.e., with this === undefined. With this change, this === req.

Suggestion: get rid of maybeCallback() entirely and use makeCallback() everywhere.

Copy link
Member Author

@BridgeAR BridgeAR Feb 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about removing it as well but makeCallback has a performance overhead compared to maybeCallback due to the wrapping.

I checked all occurrences of maybeCallback and found two more places where it was wrong right now. I fixed those in a separate commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put back the makeCallback() here? It's a needless change now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2018

Comments addressed and I also fixed two occurrences where the callback execution function was actually wrong.

New CI: https://ci.nodejs.org/job/node-test-pull-request/13062/

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a comment.

I have a mild preference for replacing maybeCallback(callback) statements with inline type checks but I know there are a few places that call it with callback || options || .... Those will be harder to replace while still keeping it succinct.

lib/fs.js Outdated

mode = mode | 0;
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put back the makeCallback() here? It's a needless change now.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2018
@BridgeAR BridgeAR requested a review from a team February 12, 2018 14:43
@joyeecheung
Copy link
Member

@BridgeAR
Copy link
Member Author

The CITGM came out green as far as I see it.

@joyeecheung
Copy link
Member

Relavent errors:

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1283/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/winston_v2_4_0/

 Log level named "log" will clash with the method "log". Consider using a different name.
 error: test messageXdebug: test messageXerror: test message
 debug: test message
 fs.js:122
     throw new errors.TypeError('ERR_INVALID_CALLBACK');
     ^
 TypeError [ERR_INVALID_CALLBACK]: Callback must be a function
     at makeCallback (fs.js:122:11)
     at Object.fs.close (fs.js:692:20)
     at read (/home/iojs/build/workspace/citgm-smoker/nodes/fedora-last-latest-x64/citgm_tmp/e8e90320-88bd-42f4-a5e1-0698c5da8e85/winston/lib/winston/common.js:415:12)
     at /home/iojs/build/workspace/citgm-smoker/nodes/fedora-last-latest-x64/citgm_tmp/e8e90320-88bd-42f4-a5e1-0698c5da8e85/winston/lib/winston/common.js:470:16
     at FSReqWrap.wrapper [as oncomplete] (fs.js:763:17)
 error: test message
 debug: test message
 npm ERR! Test failed.  See above for more details.

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1283/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/watchify_v3_10_0/

 fs.js:114
   throw new errors.TypeError('ERR_INVALID_CALLBACK');
   ^
 TypeError [ERR_INVALID_CALLBACK]: Callback must be a function
     at maybeCallback (fs.js:114:9)
     at Object.fs.writeFile (fs.js:1485:14)
     at /home/iojs/build/workspace/citgm-smoker/nodes/fedora-last-latest-x64/citgm_tmp/bc1e4bb5-05ea-4e4e-89c2-d6c1482997ca/watchify/test/bin_brfs.js:40:20
     at Socket.<anonymous> (/home/iojs/build/workspace/citgm-smoker/nodes/fedora-last-latest-x64/citgm_tmp/bc1e4bb5-05ea-4e4e-89c2-d6c1482997ca/watchify/test/bin_brfs.js:58:9)
     at Socket.emit (events.js:136:15)
     at endReadableNT (_stream_readable.js:1020:12)
     at process._tickCallback (internal/process/next_tick.js:91:19)
 npm ERR! Test failed.  See above for more details.

@joyeecheung
Copy link
Member

BTW: I don't object to this change, but we should at least get rid of the relevant errors in CITGM either by removing them or fixing them...it's not good to add more failures there.

lib/fs.js Outdated
@@ -108,46 +107,17 @@ function copyObject(source) {
return target;
}

// TODO(joyeecheung): explore how the deprecation could be solved via linting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if there is a linting rule for this...? Could not find one in https://standardjs.com/rules.html at least.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no such rule in eslint at the moment. We could of course create one and add that to eslint. I am going to open an issue there about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung if you want to get something like that in eslint, I guess we have to get some likes under my suggestion ;-)

lib/fs.js Outdated
@@ -411,16 +375,8 @@ fs.accessSync = function(path, mode) {
}
};

// fs.exists never throws even when the arguments are invalid - if there is
// a callback it would invoke it with false, otherwise it emits a warning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we throw on invalid callbacks in fs.exists now, it might make sense to throw on other invalid arguments as well...I don't feel strongly about fixing that in this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather fix that in another PR.

@BridgeAR
Copy link
Member Author

@joyeecheung thanks, seems like I did nit look close enough. I totally agree that they should be fixed. I will open patches for them.

@BridgeAR
Copy link
Member Author

@ChALkeR I guess it might be hard to check other packages but could you try to find some packages that still did not update? That way I would go ahead and open PRs for those.

@BridgeAR
Copy link
Member Author

Watchify got fixed.

The callback should run in the global scope and not in the FSReqWrap
context.
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 22, 2018

@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

The PR for winston is open quite some time now without any reaction. I also wrote @indexzero and @jcrugzz an email to please take a look at the PR but I did not get any reaction either.

Therefore I would like to progress further and go ahead and land this PR even though Winston will break without landing that PR. I am going to open a PR in CITGM to skip Winston from version >9 on.

Please take another look and give a LG for this.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 23, 2018

@BridgeAR, sorry, missed the ping. Will take a look today when I'll get to my notebook.

@BridgeAR
Copy link
Member Author

@ChALkeR well, it should apply to all async fs functions. I might of course be wrong but I can run a quick check if you like?

@ChALkeR
Copy link
Member

ChALkeR commented Feb 26, 2018

@BridgeAR I needed the exact list ;-).

I am using this one, from the docs:

fs.access
fs.appendFile
fs.chmod
fs.chown
fs.close
fs.copyFile
fs.exists
fs.fchmod
fs.fchown
fs.fdatasync
fs.fstat
fs.fsync
fs.ftruncate
fs.futimes
fs.lchmod
fs.lchown
fs.link
fs.lstat
fs.mkdir
fs.mkdtemp
fs.open
fs.read
fs.readdir
fs.readFile
fs.readlink
fs.realpath
fs.realpath.native
fs.rename
fs.rmdir
fs.stat
fs.symlink
fs.truncate
fs.unlink
fs.utimes
fs.watch
fs.watchFile
fs.write
fs.writeFile

Let me know if I missed anything ;-)

@BridgeAR
Copy link
Member Author

@ChALkeR that looks correct. I guess you want to check for further findings. That does not block the PR from landing though out of your perspective, correct? I am going to open PRs in all major findings from you anyway.

One interesting point to notice by the way: some functions actually already require a callback while others do not. So this is also going to make everything consistent.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 26, 2018

@BridgeAR Here is an updated list.

It's limited at > 10000 d/m, and it is significantly crude as it contains both false positives (e.g. some packages use promisified fs under the same name) and false negatives (I agressively excluded stuff).

This also attempts to exclude tests/examples.

No, that doesn't block this from landing on my perspective.

Click to expand

Processed:

14246196	chokidar-2.0.1.tgz/lib/fsevents-handler.js:216:        if (fd) fs.close(fd);
14246196	chokidar-2.0.1.tgz/lib/nodefs-handler.js:93:          if (fd) fs.close(fd);
9272934	typescript-2.7.1.tgz/lib/tsserver.js:91258:                    fs.close(this.fd);
6528474	winston-2.4.0.tgz/lib/winston/common.js:415:        fs.close(fd);
3545797	karma-2.0.0.tgz/lib/server.js:272:      fs.write(browserLogFile, logString + '\n')
2063361	redis-commands-1.3.1.tgz/tools/build.js:61:  fs.writeFile(commandPath, content)
675700	forever-0.15.3.tgz/lib/forever/worker.js:96:        fs.unlink(self._sockFile);
664945	react-native-0.53.0.tgz/local-cli/library/library.js:34:    fs.mkdir(libraries);
440853	promised-io-0.3.5.tgz/fs.js:52:						fs.close(fd);
288860	mimer-0.3.2.tgz/browser_build.js:14:    fs.writeFile('dist/mimer.js', data.replace('$_MIMER_DATA_LIST', jsonData));
185921	jscodeshift-0.4.0.tgz/src/Runner.js:161:              fs.write(info.fd, contents);
146375	grunt-lib-phantomjs-1.1.0.tgz/phantomjs/main.js:33:  fs.write(tmpfile, JSON.stringify(args) + '\n', 'a');
88041	grunt-contrib-jasmine-1.1.0.tgz/tasks/jasmine.js:156:      fs.unlink(options.outfile);
87513	vscode-nls-3.2.1.tgz/lib/main.js:198:                fs.mkdir(directory);
86470	react-syntax-highlighter-7.0.0.tgz/lib/build-languages-highlightjs.js:25:  fs.writeFile(path.join(__dirname, `../src/languages/hljs/${file}`), lines.join(';\n'));
86470	react-syntax-highlighter-7.0.0.tgz/lib/build-languages-highlightjs.js:38:  fs.writeFile(path.join(__dirname, '../AVAILABLE_LANGUAGES_HLJS.MD'), languageMD);
86470	react-syntax-highlighter-7.0.0.tgz/lib/build-languages-highlightjs.js:41:  fs.writeFile(path.join(__dirname, '../src/languages/hljs/index.js'), defaultExports.join(''));
86470	react-syntax-highlighter-7.0.0.tgz/lib/build-stylesheets-highlightjs.js:25:  fs.writeFile(path.join(__dirname, '../AVAILABLE_STYLES_HLJS.MD'), styleMD);
86470	react-syntax-highlighter-7.0.0.tgz/lib/build-stylesheets-highlightjs.js:26:  fs.writeFile(path.join(__dirname, '../src/styles/hljs/index.js'), defaultExports.join(''));
86470	react-syntax-highlighter-7.0.0.tgz/lib/build-stylesheets-highlightjs.js:60:    fs.writeFile(path.join(__dirname, `../src/styles/hljs/${fileWithoutCSS}.js`), 
86470	react-syntax-highlighter-7.0.0.tgz/lib/build-stylesheets-refractor.js:97:    fs.writeFile(path.join(__dirname, `../src/styles/prism/${fileWithoutCSS}.js`),
77645	emailjs-2.0.0.tgz/smtp/message.js:344:                        fs.read(fd, buffer, 0, chunk, null, read);
77645	emailjs-2.0.0.tgz/smtp/message.js:359:            fs.read(fd, buffer, 0, chunk, null, read);
77645	emailjs-2.0.0.tgz/smtp/message.js:366:      fs.open(attachment.path, 'r', opened);
41302	graphviz-0.0.8.tgz/lib/graphviz.js:77:      fs.write(info.fd, file_or_script);
40034	jest-junit-reporter-1.1.0.tgz/src/__tests__/index.spec.js:51:    fs.unlink(`${cwd}/test-report.xml`);
40034	jest-junit-reporter-1.1.0.tgz/src/__tests__/index.spec.js:52:    fs.unlink(`${cwd}/${filename}`);
40034	jest-junit-reporter-1.1.0.tgz/src/__tests__/index.spec.js:57:    fs.unlink(`${cwd}/${filename}`);

Won't fix (e.g., deprecated, promises, PhantomJS)

// Unmaintained
105636	restler-3.4.0.tgz/lib/multipartform.js:126:      			  fs.close(fd);
// Deprecated
672429	wrench-1.5.9.tgz/lib/wrench.js:327:            fs.chmod(_path.join(sourceDir, files[i]), filemode);
672429	wrench-1.5.9.tgz/lib/wrench.js:332:    fs.chmod(sourceDir, filemode);
// PhatomJS
571563	img-stats-0.5.2.tgz/lib/img-stats.js:51:      data = fs.open( filename, "r+b" ).read();
477552	jasmine-reporters-2.3.0.tgz/bin/phantomjs-testrunner.js:81:            fs.write("/dev/stdout", msg, "w");
477552	jasmine-reporters-2.3.0.tgz/bin/phantomjs-testrunner.js:203:                        fs.write(filename, output, "w");
104196	c3-0.4.20.tgz/extensions/exporter/phantom-exporter.js:26:	config = JSON.parse( fs.read('config.json') ),
104196	c3-0.4.20.tgz/extensions/exporter/phantom-exporter.js:138:	css += fs.read(config.css[i]);
92016	casperjs-1.1.4.tgz/modules/casper.js:651:        fs.write(targetPath, cu.decode(this.base64encode(url, method, data)), 'wb');
92016	casperjs-1.1.4.tgz/modules/casper.js:883:                if (!fs.exists(filePath)) {
92016	casperjs-1.1.4.tgz/modules/casper.js:1822:        if (!value || !fs.exists(value)) {
92016	casperjs-1.1.4.tgz/modules/tester.js:1448:            result.lineContents = fs.read(this.currentTestFile).split('\n')[result.line - 1].trim();
92016	casperjs-1.1.4.tgz/modules/tester.js:1628:        if (!fs.exists(path)) {
92016	casperjs-1.1.4.tgz/modules/tester.js:1699:        fs.write(filepath, exporter.getSerializedXML(), 'w');
92016	casperjs-1.1.4.tgz/samples/bbcshots.js:72:    fs.write("result.html", pageHtml, 'w');
43283	mocha-phantomjs-core-2.1.2.tgz/mocha-phantomjs-core.js:35:  output = config.file ? fs.open(config.file, 'w') : system.stdout,
43283	mocha-phantomjs-core-2.1.2.tgz/mocha-phantomjs-core.js:194:      var customReporter = fs.read(reporter)
40618	penthouse-1.4.0.tgz/lib/phantomjs/normalize-css.js:174:  var f = fs.open(options.css, 'r');
38227	fingerprintjs2-1.6.1.tgz/specs/phantomjs-testrunner.js:82:            fs.write('/dev/stdout', msg, 'w');
38227	fingerprintjs2-1.6.1.tgz/specs/phantomjs-testrunner.js:204:                        fs.write(filename, output, "w");
37323	phridge-2.0.0.tgz/lib/phantom/start.js:11:var config = JSON.parse(fs.read(configPath));
30724	phantomas-1.20.0.tgz/core/logger.js:21:		stream = fs.open(logFile, 'w');
30724	phantomas-1.20.0.tgz/extensions/har/har.js:233:			fs.write(path, dump);
30724	phantomas-1.20.0.tgz/extensions/pageSource/pageSource.js:33:		fs.write(path, phantomas.getSource(), 'w');
30724	phantomas-1.20.0.tgz/modules/analyzeCss/analyzeCss.js:227:			fs.write(path, cssContent, 'w');
21943	grunt-mocha-1.0.4.tgz/phantomjs/main.js:33:  fs.write(tmpfile, JSON.stringify(args) + '\n', 'a');
15449	phantom-html-to-pdf-0.5.5.tgz/lib/prepareScripts.js:27:                fs.writeFile(path.join(__dirname, "scripts", "serverScript.js"), serverScript.replace("$conversion", conversion).replace("$log", log));
15449	phantom-html-to-pdf-0.5.5.tgz/lib/prepareScripts.js:28:                fs.writeFile(path.join(__dirname, "scripts", "standaloneScript.js"), standaloneScript.replace("$conversion", conversion).replace("$log", log));
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/conversionScriptPart.js:112:                    var stream = fs.open(body.headerFile, "r");
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/conversionScriptPart.js:124:                    var stream = fs.open(body.footerFile, "r");
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/serverScript.js:174:                    var stream = fs.open(body.headerFile, "r");
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/serverScript.js:186:                    var stream = fs.open(body.footerFile, "r");
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/standaloneScript.js:43:    var stream = fs.open(settingsFile, "r");
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/standaloneScript.js:175:                    var stream = fs.open(body.headerFile, "r");
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/standaloneScript.js:187:                    var stream = fs.open(body.footerFile, "r");
15449	phantom-html-to-pdf-0.5.5.tgz/lib/scripts/standaloneScriptPart.js:17:    var stream = fs.open(settingsFile, "r");
11146	tus-js-client-1.4.5.tgz/bin/phantom-jasmine.js:29:var reporterScript = prelude + fs.read(__dirname + "../node_modules/jasmine/lib/reporters/console_reporter.js") + postlude;
10319	markdown-pdf-8.1.1.tgz/phantom/render.js:18:var html = fs.read(html5bpPath + '/index.html')
10319	markdown-pdf-8.1.1.tgz/phantom/render.js:20:  .replace('{{content}}', fs.read(args.in))
// Outdated - explicitely for Node.js < 0.12.
562517	sync-exec-0.6.2.tgz/js/lib/create-pipes.js:23:        fs.mkdir(dir);
65624	execSync-1.0.2.tgz/index.js:70:      fs.unlink(tempName);
// False negative
732542	swagger-server-1.0.0-alpha.18.tgz/lib/watcher.js:137:    var watcher = fs.watch(path, {persistent: false});
412942	ssh2-0.5.5.tgz/lib/agent.js:205:            fs.readFile(sockPath, readCygsocket);
346937	yeoman-generator-2.0.2.tgz/lib/util/storage.js:50:    this.fs.write(this.path, JSON.stringify(fullStore, null, '  '));
218267	q-io-1.13.5.tgz/http-apps/fs.js:145:            body: fs.open(path, options),
108481	node-watch-0.5.7.tgz/lib/has-native-recursive.js:82:  var watcher = fs.watch(parent, options);
108481	node-watch-0.5.7.tgz/lib/watch.js:282:  var watcher = fs.watch(parent, opts);
108481	node-watch-0.5.7.tgz/lib/watch.js:313:    var watcher = fs.watch(dir, opts);
69917	node-ninja-1.0.2.tgz/lib/configure.js:169:    fs.writeFile(configPath, [prefix, json, ''].join('\n'), findConfigs)
69917	node-ninja-1.0.2.tgz/lib/install.js:248:        fs.writeFile(installVersionPath, gyp.package.installVersion + '\n', deref)
60713	coffee-register-0.1.3.tgz/src/index.coffee:19:		content = fs.read(filename)
60713	coffee-register-0.1.3.tgz/lib/index.js:28:    content = fs.read(filename);
60713	coffee-register-0.1.3.tgz/lib/index.js:33:      compiledContent = fs.read(cachedFilePath);
60713	coffee-register-0.1.3.tgz/lib/index.js:40:      fs.write(cachedFilePath, compiledContent);
55213	generator-jhipster-4.14.0.tgz/generators/docker-compose/index.js:140:                    const yaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/app.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/docker-compose/index.js:176:                        const databaseYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/${database}.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/docker-compose/index.js:183:                            const cassandraClusterYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/cassandra-cluster.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/docker-compose/index.js:188:                            const cassandraMigrationYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/cassandra-migration.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/docker-compose/index.js:204:                            const clusterDbYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/${database}-cluster.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/docker-compose/index.js:223:                        const searchEngineYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/${searchEngine}.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/export-jdl/index.js:46:        this.fs.write(this.jdlFile, content);
55213	generator-jhipster-4.14.0.tgz/generators/generator-base-private.js:539:            _this.fs.write(_this.destinationPath(destination), res);
55213	generator-jhipster-4.14.0.tgz/generators/generator-base.js:2100:        if (this.fs.exists(keyStoreFile)) {
55213	generator-jhipster-4.14.0.tgz/generators/rancher-compose/index.js:131:                    const yaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/app.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/rancher-compose/index.js:173:                        const databaseYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/${database}.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/rancher-compose/index.js:182:                            const cassandraClusterYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/cassandra-cluster.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/rancher-compose/index.js:187:                            const cassandraMigrationYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/cassandra-migration.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/rancher-compose/index.js:203:                        const searchEngineYaml = jsyaml.load(this.fs.read(`${path}/src/main/docker/${searchEngine}.yml`));
55213	generator-jhipster-4.14.0.tgz/generators/utils.js:52:    args.haystack = generator.fs.read(fullPath);
55213	generator-jhipster-4.14.0.tgz/generators/utils.js:54:    generator.fs.write(fullPath, body);
55213	generator-jhipster-4.14.0.tgz/generators/utils.js:68:    let body = generator.fs.read(fullPath);
55213	generator-jhipster-4.14.0.tgz/generators/utils.js:70:    generator.fs.write(fullPath, body);
55213	generator-jhipster-4.14.0.tgz/generators/utils.js:174:            generator.fs.write(dest, body);
44556	fs.notify-0.0.4.tgz/notify.js:102:  FSWatcher = this.FSWatchers[path] = fs.watch(path);
// Already fixed
210329	glslify-6.1.0.tgz/bin.js:99:    fs.writeFile(argv.output, src)
81089	sails-0.12.14.tgz/bin/sails-console.js:145:      fs.write(fd, code + '\n');
78479	pxt-core-3.3.5.tgz/built/pxt.js:129560:                            fs.writeFile("built/web/rtl" + cssFile, result2.css, undefined);
78479	pxt-core-3.3.5.tgz/built/pxt.js:131427:                fs.writeFile(tf, langTranslations, { encoding: "utf8" }, undefined);
// Promises
130859	cp-file-5.0.0.tgz/index.js:51:					fs.utimes(dest, stats.atime, stats.mtime),
130859	cp-file-5.0.0.tgz/index.js:52:					fs.chmod(dest, stats.mode),
130859	cp-file-5.0.0.tgz/index.js:53:					fs.chown(dest, stats.uid, stats.gid)
59279	simplyimport-4.0.0-s28.tgz/lib/helpers/resolveHttpModule.coffee:26:				promiseBreak('requires download') if not fs.exists(cachedPath = Path.resolve(helpers.temp(),hash+".#{ext}"))
59279	simplyimport-4.0.0-s28.tgz/lib/helpers/resolveHttpModule.coffee:45:			unless fs.exists(cachedPath)
59279	simplyimport-4.0.0-s28.tgz/lib/task.coffee:552:			envFile = fs.read(Path.resolve context, env)
51488	windows-build-tools-2.2.1.tgz/src/utils/find-logfile.js:18:    fs.readdir(tmp)
44797	node-red-0.18.2.tgz/red/runtime/storage/localfilesystem/projects/Project.js:437:            fs.exists(fspath.join(self.path,".git","MERGE_HEAD"))
44797	node-red-0.18.2.tgz/red/runtime/storage/localfilesystem/projects/Project.js:842:            promises.push(fs.stat(fspath.join(projectPath,file)));
44797	node-red-0.18.2.tgz/red/runtime/storage/localfilesystem/projects/ssh/index.js:158:        fs.access(privateKeyFilePath, (fs.constants || fs).R_OK),
44797	node-red-0.18.2.tgz/red/runtime/storage/localfilesystem/projects/ssh/index.js:159:        fs.access(publicKeyFilePath , (fs.constants || fs).R_OK)
42694	polyserve-0.23.0.tgz/src/util/tls.ts:39:        fs.writeFile(certPath, certObj.cert),
42694	polyserve-0.23.0.tgz/src/util/tls.ts:40:        fs.writeFile(keyPath, certObj.key)
42694	polyserve-0.23.0.tgz/lib/util/tls.js:40:                    fs.writeFile(certPath, certObj.cert),
42694	polyserve-0.23.0.tgz/lib/util/tls.js:41:                    fs.writeFile(keyPath, certObj.key)
// Unknown
55274	pug-lint-2.5.0.tgz/generators/rule.js:92:    this.fs.write(path.resolve(this.destinationRoot(), 'test', 'fixtures', 'rules', this.filename + '.pug'), '//- ...');

Unprocessed:

108746	react-native-branch-2.2.4.tgz/scripts/androidUtil.js:77:  fs.unlink(path)
38052	vows-0.8.1.tgz/lib/vows/coverage/report-html.js:132:    fs.close(out);
38052	vows-0.8.1.tgz/lib/vows/coverage/report-json.js:49:        fs.close(out);
35588	rucksack-css-1.0.2.tgz/bin/cmd.js:42:      fs.writeFile(OPTS.to + '.map', result.map.toString());
33498	laggard-2.0.1.tgz/bin/cmd.js:42:      fs.writeFile(OPTS.to + '.map', result.map.toString());
31767	npm-cli-login-0.0.10.tgz/lib/login.js:120:        fs.writeFile(args.configPath, lines.join('\n') + '\n');
31339	statsd-0.8.0.tgz/servers/tcp.js:28:    config.socket && config.socket_mod && fs.chmod(config.socket, config.socket_mod);
31245	polymer-cli-1.6.0.tgz/lib/commands/build.js:167:                promises.push(mzfs.writeFile(path.join(mainBuildDirectoryName, 'polymer.json'), config.toJSON()));
31032	reify-0.14.1.tgz/node/compile-hook.js:38:      ? fs.gunzip(fs.readFile(cacheFilePath), "utf8")
31032	reify-0.14.1.tgz/node/compile-hook.js:39:      : fs.readFile(cacheFilePath, "utf8");
31032	reify-0.14.1.tgz/node/compile-hook.js:50:      ? fs.gunzip(fs.readFile(filePath), "utf8")
31032	reify-0.14.1.tgz/node/compile-hook.js:51:      : fs.readFile(filePath, "utf8");
31032	reify-0.14.1.tgz/node/utils.js:146:  const cacheFiles = cachePath === null ? null : fs.readdir(cachePath);
30092	hexo-3.5.0.tgz/lib/hexo/post.js:64:      fs.writeFile(path, content),
28674	azure-cli-0.10.17.tgz/lib/commands/batch/batch.file.js:668:                        fs.close(fd);
28674	azure-cli-0.10.17.tgz/lib/commands/batch/batch.file.js:781:                        fs.close(fd);
28674	azure-cli-0.10.17.tgz/lib/commands/batch/batch.node.js:725:                        fs.close(fd); _(); }, true)); }, true)); }); }); }); }, true)); }, true)); }, true)); }, true)); }); };
28625	lokijs-1.5.2.tgz/src/loki-incremental-adapter.js:25:      fs.writeFile(`${dir}/${coll}/${obj.$loki}.json`, JSON.stringify(obj), {
27921	appmetrics-3.1.3.tgz/headless_zip.js:38:  fs.unlink(filename);
27249	wrench-sui-0.0.3.tgz/lib/wrench.js:327:            fs.chmod(_path.join(sourceDir, files[i]), filemode);
27249	wrench-sui-0.0.3.tgz/lib/wrench.js:332:    fs.chmod(sourceDir, filemode);
27249	wrench-sui-0.0.3.tgz/lib/wrench.js:392:                    fs.unlink(file, rmFile);
27249	wrench-sui-0.0.3.tgz/lib/wrench.js:447:                                    fs.symlink(link, newFile, copyFiles);
27249	wrench-sui-0.0.3.tgz/lib/wrench.js:452:                                    fs.writeFile(newFile, data, copyFiles);
25931	backstopjs-3.1.16.tgz/capture/engine_scripts/casper/loadCookies.js:8:  if (fs.exists(cookiePath)) {
25931	backstopjs-3.1.16.tgz/capture/engine_scripts/casper/loadCookies.js:9:    cookies = JSON.parse(fs.read(cookiePath));
25931	backstopjs-3.1.16.tgz/capture/genBitmaps.js:333:      fs.write(filePath, fs.read(hiddenSelectorPath, 'b'), 'b');
25931	backstopjs-3.1.16.tgz/capture/genBitmaps.js:336:    fs.write(filePath, fs.read(selectorNotFoundPath, 'b'), 'b');
25931	backstopjs-3.1.16.tgz/capture/genBitmaps.js:354:  fs.write(comparePairsFileName, JSON.stringify(compareConfigJSON, null, 2), 'w');
25222	raygun-0.9.1.tgz/lib/raygun.offline.js:26:          fs.unlink(path.join(storage.cachePath, item));
24747	html-differ-1.3.4.tgz/lib/cli.js:81:            vfs.read(path.resolve(args.path1), 'utf-8'),
24747	html-differ-1.3.4.tgz/lib/cli.js:82:            vfs.read(path.resolve(args.path2), 'utf-8'),
24747	html-differ-1.3.4.tgz/lib/cli.js:83:            opts.config ? vfs.read(path.resolve(opts.config)) : undefined
23373	generator-talend-0.155.1.tgz/generators/app/index.js:60:		if (!this.fs.exists(this.destinationPath('.git/config'))) {
23190	qtip2-3.0.3.tgz/generate-bower.json.js:22:fs.writeFile('bower.json', JSON.stringify(bower, null, '\t'));
22545	quasar-cli-0.6.5.tgz/bin/wrappers/cordova/install.js:32:  if (qfs.symlink('../dist', www)) {
22515	phonegap-build-0.10.0.tgz/lib/phonegap-build/create/zip.js:119:            fs.rmdir(basepath);
22247	metaparser-1.0.7.tgz/index.js:34:                fs.readFile(options.source, decodeData);
20911	fs-jetpack-1.3.0.tgz/lib/utils/fs.js:33:          throw new Error("fs.exists() is deprecated");
20138	eslint-plugin-scanjs-rules-0.2.1.tgz/_tools/tests-write-to-disk.js:40:  fs.writeFile(k, s);
19938	swagger2openapi-2.11.16.tgz/testRunner.js:117:            fs.writeFile(outFile, resultStr, argv.encoding);
19756	ghost-1.21.1.tgz/core/server/ghost-server.js:66:            fs.chmod(socketValues.path, socketValues.permissions);
19492	pangyp-2.3.3.tgz/lib/configure.js:243:    fs.writeFile(configPath, [prefix, json, ''].join('\n'), findConfigs)
19492	pangyp-2.3.3.tgz/lib/install.js:284:        fs.writeFile(installVersionPath, gyp.package.installVersion + '\n', deref)
19228	bit-bin-0.12.5.tgz/src/consumer/bit-json/bit-json.js:83:      fs.writeFile(composePath(bitDir), this.toJson(), repspond);
19228	bit-bin-0.12.5.tgz/src/consumer/bit-json/consumer-bit-json.js:137:      fs.writeFile(composePath(bitDir), this.toJson(), respond);
19086	meshcentral-0.1.4-m.tgz/meshagent.js:50:        if (obj.agentUpdate != null) { obj.fs.close(obj.agentUpdate.fd); obj.agentUpdate = null; }
19086	meshcentral-0.1.4-m.tgz/meshagent.js:112:                                obj.fs.close(obj.agentUpdate.fd);
19086	meshcentral-0.1.4-m.tgz/meshagent.js:132:                            obj.fs.close(obj.agentUpdate.fd);
19086	meshcentral-0.1.4-m.tgz/meshagent.js:143:                                obj.fs.close(obj.agentUpdate.fd);
19086	meshcentral-0.1.4-m.tgz/meshcentral.js:1011:                    obj.fs.close(fd);
19086	meshcentral-0.1.4-m.tgz/webserver.js:949:                            try { obj.fs.unlink(file.path); } catch (e) { }
19086	meshcentral-0.1.4-m.tgz/webserver.js:1715:                    obj.fs.close(fd);
19017	react-native-pdf-3.0.3.tgz/index.js:138:                        RNFetchBlob.fs.unlink(tempCacheFile);
19017	react-native-pdf-3.0.3.tgz/index.js:179:            RNFetchBlob.fs.unlink(tempCacheFile);
19017	react-native-pdf-3.0.3.tgz/index.js:180:            RNFetchBlob.fs.unlink(cacheFile);
19017	react-native-pdf-3.0.3.tgz/index.js:192:                        RNFetchBlob.fs.unlink(cacheFile);
19017	react-native-pdf-3.0.3.tgz/index.js:206:                        RNFetchBlob.fs.unlink(this.path);
19017	react-native-pdf-3.0.3.tgz/index.js:227:                RNFetchBlob.fs.unlink(tempCacheFile);
19017	react-native-pdf-3.0.3.tgz/index.js:271:                        RNFetchBlob.fs.unlink(tempCacheFile);
19017	react-native-pdf-3.0.3.tgz/index.js:279:                RNFetchBlob.fs.unlink(tempCacheFile);
18881	web-ext-2.4.0.tgz/tasks/travis-pr-title-lint.js:159:        fs.stat(config), fs.access(config, fs.R_OK),
17862	droppy-7.2.0.tgz/server/log.js:66:    fs.write(logfile, stripAnsi(elems.join(" ")) + "\n");
17601	aurelia-cli-0.32.0.tgz/lib/file-system.js:10: *  fs.exists() is deprecated.
16997	cssnext-1.8.4.tgz/src/__tests__/utils/index.js:71:  fs.writeFile(fixturePath(name + ".actual"), actual)
16978	webshot-0.18.0.tgz/lib/webshot.js:100:      fs.close(obj.fd);
16978	webshot-0.18.0.tgz/lib/webshot.phantom.js:185:    var f = fs.open(site, 'r');
16347	axon-2.0.3.tgz/lib/sockets/sock.js:385:            fs.unlink(port);
15902	parcel-map-3.0.1.tgz/index.js:174:		if (outfile) fs.writeFile(outfile, JSON.stringify(result, null, 2));
15849	node-haste-2.12.0.tgz/lib/Module.js:168:        var fileContentPromise = _this4._fastfs.readFile(_this4.path);
15479	always-tail-0.2.0.tgz/index.js:27:        fs.close(block.fd);
15479	always-tail-0.2.0.tgz/index.js:164:      fs.close(self.fd); 
15479	always-tail-0.2.0.tgz/index.js:172:        fs.close(item.fd); 
14864	views-morph-12.0.3.tgz/watch.js:44:  fs.writeFile(`${file}${as === 'e2e' ? '.page.js' : '.js'}`, code)
14858	browserfs-1.4.3.tgz/scripts/get_db_credentials.ts:16:  fs.mkdir(root);
14443	slimerjs-0.10.3.tgz/src/modules/slimer-sdk/system.js:213:        stream = fs.open(file, { mode:'bw'});
14443	slimerjs-0.10.3.tgz/src/modules/slimer-sdk/system.js:251:            stdin = fs.open('/dev/stdin', 'rb');
14443	slimerjs-0.10.3.tgz/src/modules/slimer-sdk/system.js:284:            stdin = fs.open('/dev/stdin', 'rb');
14443	slimerjs-0.10.3.tgz/src/modules/slimer-sdk/webpage.js:1631:                if (!fs.exists(dir)) {
14443	slimerjs-0.10.3.tgz/src/modules/slimer-sdk/webpage.js:1635:                fs.write(file, content, "wb");
14421	svg-to-png-3.1.2.tgz/lib/processing-file.js:16:		if( !fs.exists( this.pathdir + fs.separator + this.filename ) ){
14421	svg-to-png-3.1.2.tgz/lib/processing-file.js:23:		this.imagedata = fs.read( this.pathdir + fs.separator + this.filename ) || "";
14197	ng-rollbar-2.3.8.tgz/Gruntfile.js:127:          fs.writeFile(dst, newFileContent.trim() + "\n");
13693	node-fs-extra-0.8.2.tgz/lib/copy.js:62:      destExists = fs.exists(dest),
13331	ez-streams-3.2.9.tgz/src/devices/file.ts:79:			const stat = fs.stat(p, _);
13305	embark-2.6.5.tgz/lib/pipeline/npm.js:40:            fs.unlink(dest); // Delete the file async. (But we don't check the result)
13253	tsc-1.20150623.0.tgz/bin/tsserver.js:39496:                    fs.close(this.fd);
12936	elementor-2.1.0.tgz/lib/config-helper.js:21:    fs.write(info.fd, fileContents);
12586	beyond-0.2.32.tgz/lib/helpers/cache/path/disk/item.js:48:        fs.writeFile(file, JSON.stringify(content));
12586	beyond-0.2.32.tgz/lib/helpers/cache/path/disk/item.js:68:                fs.unlink(file);
12586	beyond-0.2.32.tgz/lib/helpers/cache/path/disk/item.js:80:                    fs.unlink(file);
12586	beyond-0.2.32.tgz/lib/helpers/cache/path/disk/item.js:92:                    fs.unlink(file);
12586	beyond-0.2.32.tgz/lib/helpers/cache/path/disk/item.js:114:            fs.unlink(file);
11865	keystone-4.0.0-beta.8.tgz/server/startSocketServer.js:24:		fs.chmod(unixSocket, 0x777);
11705	maka-cli-2.8.12.tgz/lib/commands/migrate.js:17:    fs.rename('.iron', '.maka');
11703	grunticon-lib-1.2.3.tgz/lib/img-stats.js:56:			data = fs.open( filename, "r+b" ).read();
11109	ember-k-codemod-0.1.6.tgz/bin/ember-k-codemod.js:93:      fs.unlink(log);
10978	extfs-0.0.7.tgz/extfs.js:130:          fs.rmdir(searchPath);
10813	ember-modules-codemod-0.2.11.tgz/bin/ember-modules-codemod.js:87:      fs.unlink(log);
10439	atlassian-soy-cli-1.3.0.tgz/app/cli.js:130:                fs.unlink(dest);
10422	oav-0.4.32.tgz/lib/xMsExampleExtractor.js:202:                    fs.writeFile(outputExamples + exampleFileName, JSON.stringify(example, null, 2));
10422	oav-0.4.32.tgz/lib/xMsExampleExtractor.js:209:          fs.writeFile(outputSwagger, JSON.stringify(swaggerObject, null, 2));
10252	shifter-1.2.0.tgz/lib/ant.js:129:                fs.writeFile(path.join(shifter.cwd(), fileName), JSON.stringify(data, null, 4) + '\n', 'utf8');
10245	lzma-2.3.2.tgz/bin/lzma.js:125:            fs.unlink(orig);
10136	generator-npm-wp-s-theme-1.0.10.tgz/app/index.js:311:							fs.rename(filePath, './languages/' + _this.themeSlug + '.pot');
10076	omni-common-ui-0.34.0.tgz/scripts/build-bins.js:27:      fs.chmod(link, 511);

@BridgeAR
Copy link
Member Author

Thanks. I will have a look at it soon again :-)

BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 26, 2018
This reverts commit 8250bfd.

PR-URL: nodejs#18668
Refs: nodejs#12562
Refs: nodejs#12976
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 26, 2018
The callback should run in the global scope and not in the FSReqWrap
context.

PR-URL: nodejs#18668
Refs: nodejs#12562
Refs: nodejs#12976
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@BridgeAR
Copy link
Member Author

Landed in e9f2cec and c3eb3ef

@BridgeAR BridgeAR closed this Feb 26, 2018
@ChALkeR
Copy link
Member

ChALkeR commented Mar 4, 2018

@BridgeAR I split the list in #18668 (comment) in two parts for convenience. Feel free to update it, btw!

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 6, 2018

I went throw a lot of the entries in the list and opened PRs accordingly. There is a significant number of false negatives though and I added a section for those in the list above.

I am aware that there are still a lot of smaller modules that use the pattern but they do not seem to be used all that much throughout the ecosystem. Therefore I am going to stop looking into the rest if no one beats me to it.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This reverts commit 8250bfd.

PR-URL: nodejs#18668
Refs: nodejs#12562
Refs: nodejs#12976
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The callback should run in the global scope and not in the FSReqWrap
context.

PR-URL: nodejs#18668
Refs: nodejs#12562
Refs: nodejs#12976
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

This ensures that errors are actually noticed in case it is not
possible to create the directory. This will also throw an error when used with the upcoming Node.js 10.x because there is no callback.

Refs: nodejs/node#18668

[CLI][MINOR][local-cli/library/library.js] - Use sync fs.mkdir for error handling
<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook/react-native#18084

Differential Revision: D7083122

Pulled By: shergin

fbshipit-source-id: a13b64e57860e1ab2c15e0c008211dc0dca3b2d2
@BridgeAR BridgeAR deleted the revert-revert-12562 branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.