From 99713125d4fe4f202356ccb4da3f0eb4a109f707 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Thu, 5 Sep 2019 05:27:25 -0700 Subject: [PATCH 1/2] Remove "Could not start watchman" warning Historically, `watchman` was much more important with the use of in-project `tmp` directory. With that approach, we had a lot more fsevent to filter through. Now, with the move to using the system's TMPDIR, this problem does not really exist anymore. On the other hand, since the user most likely did not have `watchman` already installed on their computer, this "warning" is emitted to every new Ember user when they start the server. Since it sounded like something is broken, they would then have to spend time understanding the issue (following the link) and installing the package, which is quite involved on anything other than on OS X, all for (at this point, imo) dubious benefits. I propose that we remove this warning entirely. If we want to, in Ember CLI, we could potentially count the fsevents or profile the filtering code, then emit the warning only when we it becomes an issue. (Probably not worth it though?) The trigger for this was https://github.com/ember-learn/guides-source/pull/1038, which was caused by https://github.com/ember-learn/super-rentals-tutorial/pull/21. Since we are now capturing the real output for the tutorial, it also captured the warning (the omission previously was basically a "bug"). It felt like _something_ need to be said about it in the tutorial text, since it looks like something went wrong. The options are: 1. Pre-install `watchman` on the build server, so the warning won't show up 2. Teach the readers to install `watchman` 3. Instruct the readers to ignore the warning (1) feels like cheating without also doing (2). (2) is tricky on anything other than OS X (i.e. Linux). (3) is probably the best option, but that begs the question, why did we have the warning in the first place then? After a brief discussion on Discord, it seems like maybe the time for which this warning is necessary has past, and even core team members routinely use Ember without `watchman` installed, so removing it seemed appropriate. * * * Since the `watchmanSupportsPlatform` option is not used by anymore, I have removed it. I would think that it would be used to skip certain checks, but it is not. If that was a mistake I can bring it back. --- README.md | 3 +-- lib/index.js | 21 +++++---------------- test/index_test.js | 47 +++++----------------------------------------- 3 files changed, 11 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index 371a4eb..9d1a0e3 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,6 @@ sane(root, options); ```js new WatchDetector({ ui: /* console-ui instance */, - fs: /* fs instance */, - watchmanSupportsPlatform: true | false /* defaults to no for windows */ + fs: /* fs instance */ }); ``` diff --git a/lib/index.js b/lib/index.js index cde09bb..ee421a1 100644 --- a/lib/index.js +++ b/lib/index.js @@ -2,7 +2,6 @@ const quickTemp = require('quick-temp'); const Promise = require('rsvp').Promise; -const WATCHMAN_INFO = 'Visit https://ember-cli.com/user-guide/#watchman for more info.'; const semver = require('semver'); const SilentError = require('silent-error'); @@ -11,6 +10,7 @@ const WATCHMAN = 'watchman'; const NODE = 'node'; const EVENTS = 'events'; const POSSIBLE_WATCHERS = [POLLING, WATCHMAN, NODE, EVENTS]; +const WATCHMAN_INFO = 'Visit https://ember-cli.com/user-guide/#watchman for more info.'; const debug = require('heimdalljs-logger')('ember-cli:watcher'); @@ -44,12 +44,6 @@ class WatchDetector { constructor(options) { this.childProcess = options.childProcess || require('child-process'); - if ('watchmanSupportsPlatform' in options) { - this.watchmanSupportsPlatform = options.watchmanSupportsPlatform; - } else { - this.watchmanSupportsPlatform = process.platform !== "win32"; - } - this.fs = options.fs || require('fs'); this.root = options.root; @@ -188,6 +182,10 @@ class WatchDetector { ) { // if there was an initial preference, but we had to fall back inform the user. this.ui.writeLine(`was unable to use: "${original}", fell back to: "${actual.watcher}"`); + + if (original === WATCHMAN) { + this.ui.writeLine(WATCHMAN_INFO); + } } return actual; } @@ -248,15 +246,6 @@ class WatchDetector { return result; } catch (reason) { debug.info('detecting watchman failed %o', reason); - - if (this.watchmanSupportsPlatform) { - // don't bother telling windows users watchman detection failed, that is - // until watchman is legit on windows. - } else { - this.ui.writeLine('Could not start watchman'); - this.ui.writeLine(WATCHMAN_INFO); - } - result.watcher = NODE; return result; } diff --git a/test/index_test.js b/test/index_test.js index cca9521..92824c9 100644 --- a/test/index_test.js +++ b/test/index_test.js @@ -83,7 +83,6 @@ describe('WatchDetector', function() { expect(option.watchmanInfo).to.have.property('version'); expect(option.watchmanInfo).to.have.property('canNestRoots'); expect(option.watchmanInfo).to.have.property('enabled', true); - expect(ui.output).not.to.match(/Could not start watchman/); expect(ui.output).not.to.match(/fell back to: "node"/); expect(ui.output).not.to.match(/Visit https:\/\/ember-cli.com\/user-guide\/#watchman/); }); @@ -95,7 +94,7 @@ describe('WatchDetector', function() { }; }); - it('false back to node if it can', function() { + it('falls back to node if it can', function() { fs.watch = function() { return { close() {} }; }; @@ -105,12 +104,11 @@ describe('WatchDetector', function() { expect(option).to.have.property('watcher', 'node'); expect(option.watchmanInfo).to.have.property('version'); expect(option.watchmanInfo).to.have.property('canNestRoots'); - expect(ui.output).to.match(/Could not start watchman/); expect(ui.output).to.match(/fell back to: "node"/); expect(ui.output).to.match(/Visit https:\/\/ember-cli.com\/user-guide\/#watchman/); }); - it('false back to polling if node does not work', function() { + it('falls back to polling if node does not work', function() { fs.watch = function() { throw new Error('something went wrong'); }; @@ -120,7 +118,6 @@ describe('WatchDetector', function() { expect(option.watchmanInfo).to.have.property('enabled', false); expect(option.watchmanInfo).to.have.property('version'); expect(option.watchmanInfo).to.have.property('canNestRoots'); - expect(ui.output).to.match(/Could not start watchman/); expect(ui.output).to.match(/fell back to: "polling"/); expect(ui.output).to.match(/Visit https:\/\/ember-cli.com\/user-guide\/#watchman/); }); @@ -149,7 +146,7 @@ describe('WatchDetector', function() { expect(ui.output).to.eql(''); }); - it('false back to polling if watch fails', function() { + it('falls back to polling if watch fails', function() { fs.watch = function() { throw new Error('OMG'); }; @@ -175,38 +172,6 @@ describe('WatchDetector', function() { }); describe('#checkWatchman', function() { - describe('watchmanSupportsPlatform', function() { - it('true: hides the "watchman not found, falling back to XYZ message"', function() { - subject.watchmanSupportsPlatform = true; - - childProcess.execSync = function() { - throw new Error(); - }; - fs.watch = function() { - return { close() {} }; - }; - - let result = subject.checkWatchman(); - expect(result).to.have.property('watcher', 'node'); - expect(ui.output).to.eql(''); - }); - - it('false: shows the "watchman not found, falling back to XYZ message"', function() { - subject.watchmanSupportsPlatform = false; - fs.watch = function() { - return { close() {} }; - }; - - childProcess.execSync = function() { - throw new Error(); - }; - - let result = subject.checkWatchman(); - expect(result).to.have.property('watcher', 'node'); - expect(ui.output).to.match(/Could not start watchman/); - expect(ui.output).to.match(/Visit https:\/\/ember-cli.com\/user-guide\/#watchman/); - }); - }); it('prefers watchman if everything appears to be good', function() { childProcess.execSync = function() { return '{"version":"3.0.0"}'; @@ -214,14 +179,13 @@ describe('WatchDetector', function() { let preference = subject.checkWatchman(); expect(preference).to.have.property('watcher', 'watchman'); - expect(ui.output).to.not.match(/Could not start watchman/); expect(ui.output).to.not.match(/falling back to NodeWatcher/); expect(ui.output).to.not.match(/ember-cli\.com\/user-guide\/#watchman/); expect(ui.output).to.not.match(/Looks like you have a different program called watchman/); expect(ui.output).to.not.match(/Invalid watchman found/); }); - describe('fallse back to NODE', function() { + describe('falls back to NODE', function() { let iff = it; iff('the exec rejects', function() { @@ -231,8 +195,7 @@ describe('WatchDetector', function() { let preference = subject.checkWatchman(); expect(preference).to.have.property('watcher', 'node'); - expect(ui.output).to.match(/Could not start watchman/); - expect(ui.output).to.match(/ember-cli\.com\/user-guide\/#watchman/); + expect(ui.output).to.not.match(/ember-cli\.com\/user-guide\/#watchman/); }); iff("the `watchman version` doesn't parse", function() { From e36946158e1cbc53759e6b8e9e388484386ab764 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Thu, 5 Sep 2019 06:26:29 -0700 Subject: [PATCH 2/2] Drop support for Node 4 and Node 6 --- .travis.yml | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index f0d2774..c7268cd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,8 +1,8 @@ language: node_js node_js: - - "4" - - "6" - "8" + - "10" + - "lts/*" - "stable" sudo: false diff --git a/package.json b/package.json index a841dc3..d933bea 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,7 @@ "sinon-chai": "^2.14.0" }, "engines": { - "node": ">= 4" + "node": ">= 8" }, "dependencies": { "heimdalljs-logger": "^0.1.9",