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

Use duck-typing over other types of typing #132

Merged
merged 2 commits into from
Jul 18, 2017
Merged

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Jul 18, 2017

This will allow Futures of incompatible versions, Futures spawned in other runtime-context (vm.run*), or even "custom made" Futures to be consumed by this function.

@Avaq Avaq requested a review from davidchambers July 18, 2017 13:04
@codecov-io
Copy link

codecov-io commented Jul 18, 2017

Codecov Report

Merging #132 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #132   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          48     48           
  Lines        1097   1098    +1     
=====================================
+ Hits         1097   1098    +1
Impacted Files Coverage Δ
src/after.js 100% <ø> (ø) ⬆️
src/core.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba73ba8...377e345. Read the comment docs.

@@ -8,7 +8,7 @@ function After$race(other){
? other
: isNever(other)
? this
: (other instanceof After || other instanceof RejectAfter)
: typeof other._time === 'number'
Copy link
Member Author

Choose a reason for hiding this comment

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

Good old duck-typing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these the only two occurrences of instanceof in the project?

Copy link
Member Author

@Avaq Avaq Jul 18, 2017

Choose a reason for hiding this comment

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

Technically, no. It's used in some places (like isFuture) to determine with high performance whether a value is what we want it to be, but these are the only occurrences left where there are no or conditions for the value to be identified as such.

@Avaq Avaq changed the title Avoid the use of instanceof in After#race Use duck-typing over other types of typing Jul 18, 2017
function Never(){}
function Never(){
this._isNever = true;
}
Copy link
Member Author

@Avaq Avaq Jul 18, 2017

Choose a reason for hiding this comment

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

The reason I'm setting it in the constructor, rather than on the prototype, is that it will cause inspect to provide slightly more insightful output (Future { _isNever: true } vs Future {}).

@Avaq Avaq merged commit eca4f7f into master Jul 18, 2017
@Avaq Avaq deleted the avaq/ducktype-after branch July 18, 2017 15:15
Avaq added a commit that referenced this pull request Jul 18, 2017
* #128 The performance of transforming Futures
  has improved significantly.
* #131 Future.parallel has been made stack-safe.
* #131 Future.parallel now guarantees execution order.
* #132 Improve version interoperability.

Note that 6.3.x futures will not be compatible with
6.2.x Futures, so when upgrading, make sure to
upgrade all producers of Future instances.

To avoid having to do this with future releases,
I recommend setting Fluture as a devDependency
if you are maintaining a library which exposes
Future instances to its users.
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.

None yet

3 participants