Drop support for "primitive" dispatching #277

Avaq · 2018-08-29T06:38:54Z

A while ago, I added sanctuary-type-classes as a dependency to Fluture,
primarily to gain its fantastic toString implementation. As a
side-effect, Fluture got to use its dispatcher implementations for ap,
map, bimap, chain, and alt.

Now that sanctuary-show has the toString logic, there is little to gain
from including the entirety of sanctuary-type-classes for dispatching
to FL methods.

The gains were:

Some of its logic did not have to be reimplemented. Though as shown
by this diff, not that much.
Fluture dispatchers could be used on primitive types
(like Fluture.map(f, [1, 2, 3])). I believe this functionality was
rarely, if ever, used. Furthermore, TypeScript users were unable to
use it out of the box because of the limitations of TypeScript.

The losses however:

A fairly big increase of the bundle size, which as of recent I have
been working to reduce.
A minor hit on the performance of these dispatchers.
The management cost of an additional dependency.
Breaking changes to sanctuary-type-classes primitive dispatching would
propagate to a breaking change in Fluture which was confusing to users
(see https://git.io/fAGsJ#issuecomment-355616168 for example).

codecov · 2018-08-29T06:40:38Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #277   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          48     48           
  Lines         984    990    +6     
  Branches      210    215    +5     
=====================================
+ Hits          984    990    +6

Impacted Files	Coverage Δ
src/internal/const.mjs	`100% <ø> (ø)`	⬆️
src/dispatchers/chain.mjs	`100% <100%> (ø)`	⬆️
src/dispatchers/alt.mjs	`100% <100%> (ø)`	⬆️
src/dispatchers/map.mjs	`100% <100%> (ø)`	⬆️
src/dispatchers/bimap.mjs	`100% <100%> (ø)`	⬆️
src/internal/predicates.mjs	`100% <100%> (ø)`	⬆️
src/dispatchers/ap.mjs	`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 7130b91...fa7f385. Read the comment docs.

Avaq · 2018-08-29T06:48:56Z

Performance benchmarking results:

$ npm run bench -- --match '*{def,run}.transform*'

index.cjs.mjs → index.js...
created index.js in 318ms
Running benchmarks: fluture...

┌────────────────────────────────────────────────┬────────────────────────────┬─────────────────────────────┬────────┬──────────┬───┐
│ suite                                          │ Old                        │ New                         │ diff   │ change   │ α │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ def.transform.ap                               │ 568,475 Hz ±0.59% (n 81)   │ 7,752,071 Hz ±0.46% (n 92)  │ 086.3% │ +1263.7% │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ def.transform.map                              │ 2,189,079 Hz ±0.71% (n 84) │ 14,907,675 Hz ±0.74% (n 91) │ 074.4% │ +581.0%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ def.transform.chain                            │ 780,575 Hz ±0.83% (n 87)   │ 7,656,824 Hz ±2.01% (n 84)  │ 081.5% │ +880.9%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.sync.ap                          │ 415,913 Hz ±2.86% (n 86)   │ 1,621,023 Hz ±2.31% (n 88)  │ 059.2% │ +289.8%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.sync.map                         │ 1,139,632 Hz ±3.30% (n 86) │ 2,500,284 Hz ±7.12% (n 89)  │ 037.4% │ +119.4%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.sync.chain.one                   │ 584,580 Hz ±1.90% (n 94)   │ 1,978,296 Hz ±4.61% (n 89)  │ 054.4% │ +238.4%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.sync.chain.many                  │ 855 Hz ±0.93% (n 93)       │ 5,910 Hz ±2.42% (n 95)      │ 074.7% │ +591.3%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.async.ap                         │ 181,705 Hz ±0.82% (n 78)   │ 262,019 Hz ±1.03% (n 80)    │ 018.1% │ +044.2%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.async.map                        │ 364,360 Hz ±0.75% (n 83)   │ 458,573 Hz ±0.88% (n 80)    │ 011.4% │ +025.9%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.async.chain.one                  │ 196,151 Hz ±0.76% (n 78)   │ 271,026 Hz ±0.94% (n 79)    │ 016.0% │ +038.2%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.async.chain.many                 │ 4,137 Hz ±0.61% (n 88)     │ 6,977 Hz ±0.77% (n 87)      │ 025.6% │ +068.6%  │ ✓ │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.parallel.async.race.fast-vs-slow │ 237,990 Hz ±0.92% (n 79)   │ 230,518 Hz ±1.84% (n 78)    │ 001.6% │ -003.1%  │   │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.parallel.async.race.slow-vs-fast │ 247,323 Hz ±3.92% (n 80)   │ 240,923 Hz ±1.29% (n 79)    │ 001.3% │ -002.6%  │   │
├────────────────────────────────────────────────┼────────────────────────────┼─────────────────────────────┼────────┼──────────┼───┤
│ run.transform.parallel.async.race.slow-vs-slow │ 817 Hz ±1.08% (n 82)       │ 812 Hz ±1.24% (n 79)        │ 000.4% │ -000.7%  │   │
└────────────────────────────────────────────────┴────────────────────────────┴─────────────────────────────┴────────┴──────────┴───┘

davidchambers · 2018-08-29T13:58:19Z

src/internal/predicates.mjs

@@ -29,3 +31,27 @@ export function isIterator(i){
 export function isArray(x){
  return Array.isArray(x);
 }
+
+export function hasMethod(method, x){
+  return Boolean(x) && typeof x[method] === 'function';


Why Boolean(x) rather than x != null? It seems wrong to me:

> (function (method, x) { return Boolean (x) && typeof x[method] === 'function'; } ('concat', 'xyz')) true > (function (method, x) { return Boolean (x) && typeof x[method] === 'function'; } ('concat', '')) false

That is a good point. I've been using Boolean to check if something is "accessible", but I hadn't considered strings, numbers and Booleans.

null and undefined are the only “inaccessible” values, as far as I'm aware.

davidchambers · 2018-08-29T13:59:22Z

src/internal/predicates.mjs

+}
+
+export function isFunctor(x){
+  return hasMethod(FL.map, x);


What about [], {}, Math.abs, and so on?

Sadly, those don't have Fantasy Land implementations, and are exactly what I'm dropping support for with this commit (referring to it as "primitive dispatching"). Users who want to treat these values as Functors can use (and probably already are using) Ramda or Sanctuary. map and friends in Fluture can be thought of as operating on Futures and ConcurrentFutures (although they can work with any FL type).

Fluture currently supports it through sanctuary-type-classes, but at too great a cost. I am trying to shrink the Fluture core down to the essentials, and would like to integrate with Sanctuary at a higher level.

davidchambers · 2018-08-29T14:00:42Z

src/internal/const.mjs

  bimap: 'fantasy-land/bimap',
  chain: 'fantasy-land/chain',
  chainRec: 'fantasy-land/chainRec',
-  ap: 'fantasy-land/ap',
+  map: 'fantasy-land/map',
  of: 'fantasy-land/of',
  zero: 'fantasy-land/zero'
 };


Consider making this mapping exhaustive even if some of the values are never referenced.

davidchambers · 2018-08-30T09:11:54Z

src/internal/predicates.mjs

+import {FL} from './const';
+
+export function isAccessible(x){
+  return x !== null && x !== undefined;


You could use x != null here. This is such a common idiom that I don't consider isAccessible warranted.

A while ago, I added sanctuary-type-classes as a dependency to Fluture, primarily to gain its fantastic toString implementation. As a side-effect, Fluture got to use its dispatcher implementations for ap, map, bimap, chain, and alt. Now that sanctuary-show has the toString logic, there is little to gain from including the entirety of sanctuary-type-classes for dispatching to FL methods. The gains were: - Some of its logic did not have to be reimplemented. Though as shown by this diff, not that much. - Fluture dispatchers could be used on primitive types (like Fluture.map(f, [1, 2, 3])). I believe this functionality was rarely, if ever, used. Furthermore, TypeScript users were unable to use it out of the box because of the limitations of TypeScript. The losses however: - A fairly big increase of the bundle size, which as of recent I have been working to reduce. - A minor hit on the performance of these dispatchers. - The management cost of an additional dependency. - Breaking changes to sanctuary-type-classes primitive dispatching would propagate to a breaking change in Fluture which was confusing to users (see https://git.io/fAGsJ#issuecomment-355616168 for example).

Breaking changes - #276 The "module" package file has been renamed from index.mjs.js to index.mjs. - #277 It is no longer possible to use Future.ap, Future.map, Future.bimap, Future.chain, or Future.alt on non-Fantasy Land types. If you were using any of these functions from Fluture to treat JavaScript's native types as Fantasy Land algebraic types, you should migrate those call-sites to use the Ramda or Sanctuary exported dispatchers instead. - #281 Future.finally no longer runs the cleanup Future when it is cancelled. If you rely on this behaviour, I urge you to rewrite the relevant code with Future.hook (even if you don't upgrade Fluture). New features - #276 The Fluture source is now loadable by Node's experimental module loader, and the esm loader (https://github.com/standard-things/esm). - #280 The Future.of function now has an alias 'resolve', which is exported statically as well as exposed as a property on the Future constructor. Bug fixes and improvements - #277 Fluture no longer depends on sanctuary-type-classes, reducing bundle size. - #277 Performance of Fantasy Land dispatchers greatly improved. - #281 The undesired consequences of the cancellation-related behaviour in Fluture.finally are gone now that the behaviour has been removed.

Avaq added the breaking label Aug 29, 2018

Avaq requested a review from davidchambers August 29, 2018 06:38

Avaq force-pushed the avaq/simple-dispatch branch from fcd94b0 to 4eb5929 Compare August 29, 2018 06:40

Avaq force-pushed the avaq/simple-dispatch branch from 4eb5929 to e038ab6 Compare August 29, 2018 06:47

Avaq force-pushed the avaq/simple-dispatch branch 3 times, most recently from adc6461 to eecc560 Compare August 29, 2018 10:34

davidchambers reviewed Aug 29, 2018

View reviewed changes

Avaq force-pushed the avaq/simple-dispatch branch 3 times, most recently from 70aecd5 to 04d1de2 Compare August 30, 2018 06:21

davidchambers reviewed Aug 30, 2018

View reviewed changes

davidchambers approved these changes Aug 30, 2018

View reviewed changes

Fix treating Booleans, Strings, and Numbers as inaccessible values

66864aa

Avaq force-pushed the avaq/simple-dispatch branch from 04d1de2 to 6229316 Compare August 30, 2018 11:12

Avaq force-pushed the avaq/simple-dispatch branch from 6229316 to fa7f385 Compare August 30, 2018 11:13

Avaq merged commit db91a2e into master Aug 30, 2018

Avaq deleted the avaq/simple-dispatch branch August 30, 2018 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for "primitive" dispatching #277

Drop support for "primitive" dispatching #277

Avaq commented Aug 29, 2018 •

edited

Loading

codecov bot commented Aug 29, 2018 •

edited

Loading

Avaq commented Aug 29, 2018 •

edited

Loading

davidchambers Aug 29, 2018

Avaq Aug 29, 2018

davidchambers Aug 29, 2018

davidchambers Aug 29, 2018

Avaq Aug 29, 2018

davidchambers Aug 29, 2018

davidchambers Aug 30, 2018

Drop support for "primitive" dispatching #277

Drop support for "primitive" dispatching #277

Conversation

Avaq commented Aug 29, 2018 • edited Loading

codecov bot commented Aug 29, 2018 • edited Loading

Codecov Report

Avaq commented Aug 29, 2018 • edited Loading

davidchambers Aug 29, 2018

Choose a reason for hiding this comment

Avaq Aug 29, 2018

Choose a reason for hiding this comment

davidchambers Aug 29, 2018

Choose a reason for hiding this comment

davidchambers Aug 29, 2018

Choose a reason for hiding this comment

Avaq Aug 29, 2018

Choose a reason for hiding this comment

davidchambers Aug 29, 2018

Choose a reason for hiding this comment

davidchambers Aug 30, 2018

Choose a reason for hiding this comment

Avaq commented Aug 29, 2018 •

edited

Loading

codecov bot commented Aug 29, 2018 •

edited

Loading

Avaq commented Aug 29, 2018 •

edited

Loading