refactor(filelist): rename promise -> lastCompletedRefresh and remove… #3060

johnjbarton · 2018-06-20T17:01:38Z

… unused promise

lusarz · 2018-06-20T17:24:07Z

lib/file-list.js

@@ -131,16 +131,16 @@ class FileList {

  // Check if we are currently refreshing
  _isRefreshing () {
-    return this._refreshing.isPending()
+    return this._refreshing && this._refreshing.isPending()


_isRefreshing method is not used anywhere, we can remove it.

I was working off HEAD and forgot about your changes to this file.

No problem, just wanted to mention it :)

lusarz · 2018-06-20T17:26:19Z

lib/file-list.js

  }

  // Do the actual work of refreshing
  _refresh () {
    const buckets = this.buckets
    const matchedFiles = new Set()

-    let promise
-    promise = Promise.map(this._patterns, (patternObject) => {
+    let lastCompletedRefresh = this._refreshing


Why we need it ?

Please read the comment below ;-)

Do you think it's possible to write unit tests that fails when you replace let lastCompletedRefresh = this._refreshing with let lastCompletedRefresh - like it was before ?

Well maybe I'm confused. To me

let lastCompletedRefresh = Promise.map(this._patterns, (patternObject) => {...}) .then(() => { if (this._refreshing !== lastCompletedRefresh) { return this._refreshing } }) ...

means that once this statement executes, lastCompletedRefresh is set to the promise returned by the RHS. In the promise callback the value is unambiguous. However this line will cause the test to fail with ReferenceError: lastCompletedRefresh is not defined.
If we write

let lastCompletedRefresh = null lastCompletedRefresh = Promise.map(this._patterns, (patternObject) => {...}) .then(() => { if (lastCompletedRefresh === null) { throw new Error('how can this be') } }) ...

Then one of the unit tests throws "how can this be". I don't understand how the then call back can run before the promise assignment.

So, no, but I also don't understand the unit tests behavior now.

(This long chain of promises at different levels is to hard to read. IMO it should be refactored to make the chains clearer).

Then one of the unit tests throws "how can this be". I don't understand how the then call back can run before the promise assignment.

Because of something like this, which causes promise callbacks to be executed synchronously (and therefore before the assignment).

I'm working on simplifying this piece now.

… unused promise

All supported Node versions support native promises and async/await, current code has a minimal usage of Bluebird-specific methods, so dropping a third-party library in favor of native Promises. Tests were using `Promise.setScheduler()` from Bluebird, which allowed to customize how Promises are scheduled and made test work very different from the real code, which created tricky issues (like karma-runner#3060 (comment)). Affected tests were updated to not rely on `Promise.setScheduler()`, which improved readability in some situations. Also removed some test helpers as they are not necessary and not used anymore. BREAKING CHANGE: Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core

All supported Node versions support native promises and async/await, current code has a minimal usage of Bluebird-specific methods, so dropping a third-party library in favor of native Promises. Tests were using `Promise.setScheduler()` from Bluebird, which allowed to customize how Promises are scheduled and made test work very different from the real code, which created tricky issues (like #3060 (comment)). Affected tests were updated to not rely on `Promise.setScheduler()`, which improved readability in some situations. Also removed some test helpers as they are not necessary and not used anymore. BREAKING CHANGE: Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core

# [5.0.0](v4.4.1...v5.0.0) (2020-04-09) ### Bug Fixes * install semantic-release as a regular dev dependency ([#3455](#3455)) ([1eaf35e](1eaf35e)) * **ci:** echo travis env that gates release after_success ([#3446](#3446)) ([b8b2ed8](b8b2ed8)) * **ci:** poll every 10s to avoid rate limit. ([#3388](#3388)) ([91e7e00](91e7e00)) * **middleware/runner:** handle file list rejections ([#3400](#3400)) ([80febfb](80febfb)), closes [#3396](#3396) [#3396](#3396) * **server:** cleanup import of the removed method ([#3439](#3439)) ([cb1bcbf](cb1bcbf)) * **server:** createPreprocessor was removed ([#3435](#3435)) ([5c334f5](5c334f5)) * **server:** detection new MS Edge Chromium ([#3440](#3440)) ([7166ce2](7166ce2)) * **server:** replace optimist on yargs lib ([#3451](#3451)) ([ec1e69a](ec1e69a)), closes [#2473](#2473) * **server:** Report original error message ([#3415](#3415)) ([79ee331](79ee331)), closes [#3414](#3414) ### Code Refactoring * use native Promise instead of Bluebird ([#3436](#3436)) ([33a069f](33a069f)), closes [/github.com//pull/3060#discussion_r284797390](https://github.com//github.com/karma-runner/karma/pull/3060/issues/discussion_r284797390) ### Continuous Integration * drop node 8, adopt node 12 ([#3430](#3430)) ([a673aa8](a673aa8)) ### Features * **docs:** document `DEFAULT_LISTEN_ADDR` constant ([#3443](#3443)) ([057d527](057d527)), closes [#2479](#2479) * **karma-server:** added log to the server.js for uncaught exception ([#3399](#3399)) ([adc6a66](adc6a66)) * **preprocessor:** obey Pattern.isBinary when set ([#3422](#3422)) ([708ae13](708ae13)), closes [#3405](#3405) ### BREAKING CHANGES * Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core * **server:** Deprecated createPreprocessor removed, karma-browserify < 7 version doesn't work * no more testing on node 8.

All supported Node versions support native promises and async/await, current code has a minimal usage of Bluebird-specific methods, so dropping a third-party library in favor of native Promises. Tests were using `Promise.setScheduler()` from Bluebird, which allowed to customize how Promises are scheduled and made test work very different from the real code, which created tricky issues (like karma-runner#3060 (comment)). Affected tests were updated to not rely on `Promise.setScheduler()`, which improved readability in some situations. Also removed some test helpers as they are not necessary and not used anymore. BREAKING CHANGE: Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core

# [5.0.0](karma-runner/karma@v4.4.1...v5.0.0) (2020-04-09) ### Bug Fixes * install semantic-release as a regular dev dependency ([karma-runner#3455](karma-runner#3455)) ([1eaf35e](karma-runner@1eaf35e)) * **ci:** echo travis env that gates release after_success ([karma-runner#3446](karma-runner#3446)) ([b8b2ed8](karma-runner@b8b2ed8)) * **ci:** poll every 10s to avoid rate limit. ([karma-runner#3388](karma-runner#3388)) ([91e7e00](karma-runner@91e7e00)) * **middleware/runner:** handle file list rejections ([karma-runner#3400](karma-runner#3400)) ([80febfb](karma-runner@80febfb)), closes [karma-runner#3396](karma-runner#3396) [karma-runner#3396](karma-runner#3396) * **server:** cleanup import of the removed method ([karma-runner#3439](karma-runner#3439)) ([cb1bcbf](karma-runner@cb1bcbf)) * **server:** createPreprocessor was removed ([karma-runner#3435](karma-runner#3435)) ([5c334f5](karma-runner@5c334f5)) * **server:** detection new MS Edge Chromium ([karma-runner#3440](karma-runner#3440)) ([7166ce2](karma-runner@7166ce2)) * **server:** replace optimist on yargs lib ([karma-runner#3451](karma-runner#3451)) ([ec1e69a](karma-runner@ec1e69a)), closes [karma-runner#2473](karma-runner#2473) * **server:** Report original error message ([karma-runner#3415](karma-runner#3415)) ([79ee331](karma-runner@79ee331)), closes [karma-runner#3414](karma-runner#3414) ### Code Refactoring * use native Promise instead of Bluebird ([karma-runner#3436](karma-runner#3436)) ([33a069f](karma-runner@33a069f)), closes [/github.com/karma-runner/pull/3060#discussion_r284797390](https://github.com//github.com/karma-runner/karma/pull/3060/issues/discussion_r284797390) ### Continuous Integration * drop node 8, adopt node 12 ([karma-runner#3430](karma-runner#3430)) ([a673aa8](karma-runner@a673aa8)) ### Features * **docs:** document `DEFAULT_LISTEN_ADDR` constant ([karma-runner#3443](karma-runner#3443)) ([057d527](karma-runner@057d527)), closes [karma-runner#2479](karma-runner#2479) * **karma-server:** added log to the server.js for uncaught exception ([karma-runner#3399](karma-runner#3399)) ([adc6a66](karma-runner@adc6a66)) * **preprocessor:** obey Pattern.isBinary when set ([karma-runner#3422](karma-runner#3422)) ([708ae13](karma-runner@708ae13)), closes [karma-runner#3405](karma-runner#3405) ### BREAKING CHANGES * Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core * **server:** Deprecated createPreprocessor removed, karma-browserify < 7 version doesn't work * no more testing on node 8.

lusarz reviewed Jun 20, 2018

View reviewed changes

johnjbarton force-pushed the extra-promise branch from 2c8082f to 9cf8b14 Compare June 21, 2018 00:08

refactor(filelist): rename promise -> lastCompletedRefresh and remove…

a13ada4

… unused promise

johnjbarton force-pushed the extra-promise branch from 9cf8b14 to a13ada4 Compare June 21, 2018 00:12

lusarz approved these changes Jun 22, 2018

View reviewed changes

johnjbarton merged commit 7617279 into karma-runner:master Jul 17, 2018

devoto13 mentioned this pull request Feb 28, 2020

refactor: use native Promise instead of Bluebird #3436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(filelist): rename promise -> lastCompletedRefresh and remove… #3060

refactor(filelist): rename promise -> lastCompletedRefresh and remove… #3060

johnjbarton commented Jun 20, 2018

lusarz Jun 20, 2018

johnjbarton Jun 20, 2018

lusarz Jun 20, 2018

lusarz Jun 20, 2018

johnjbarton Jun 21, 2018

lusarz Jun 21, 2018

johnjbarton Jun 21, 2018

devoto13 May 16, 2019

refactor(filelist): rename promise -> lastCompletedRefresh and remove… #3060

refactor(filelist): rename promise -> lastCompletedRefresh and remove… #3060

Conversation

johnjbarton commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment