Open OliverJAsh opened 4 years ago
@OliverJAsh thanks for raising this.
In your experience, does tree shaking work correctly when we use the * as
method?
I just tried:
import * as O from 'fp-ts/es6/Option';
and compared with importing just 5 individual functions { fromEither, map as optionMap, getOrElse, alt, some } from
. I see no difference in resulting bundle size (using webpack-bundle-analyzer
). I even looked at the minified and uglified javascript, and it looks to me that many other (Option
) functions are also bundled.
@wmaurer Have you enabled mode: 'production'
(in webpack)?
I did some testing on this awhile ago: https://github.com/OliverJAsh/tree-shaking-test/blob/587a8776022b6b4fa27b9902527af19a87acd670/src/index.js.
Judging by the output, it does seem to work:
/***/ "./node_modules/fp-ts/es6/Option.js":
/*!******************************************!*\
!*** ./node_modules/fp-ts/es6/Option.js ***!
\******************************************/
/*! exports provided: URI, none, some, isSome, isNone, fold, fromNullable, toNullable, toUndefined, getOrElse, elem, exists, fromPredicate, tryCatch, getLeft, getRight, getRefinement, mapNullable, getShow, getEq, getOrd, getApplySemigroup, getApplyMonoid, getFirstMonoid, getLastMonoid, getMonoid, option, alt, ap, apFirst, apSecond, chain, chainFirst, duplicate, extend, filter, filterMap, flatten, foldMap, map, partition, partitionMap, reduce, reduceRight, compact, separate, fromEither */
/*! exports used: fromNullable */
/***/ (function(module, __webpack_exports__, __webpack_require__) {
"use strict";
/* unused harmony export URI */
/* unused harmony export none */
/* unused harmony export some */
/* unused harmony export isSome */
/* unused harmony export isNone */
/* unused harmony export fold */
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "a", function() { return fromNullable; });
/* unused harmony export toNullable */
/* unused harmony export toUndefined */
/* unused harmony export getOrElse */
/* unused harmony export elem */
/* unused harmony export exists */
/* unused harmony export fromPredicate */
/* unused harmony export tryCatch */
/* unused harmony export getLeft */
/* unused harmony export getRight */
/* unused harmony export getRefinement */
/* unused harmony export mapNullable */
/* unused harmony export getShow */
/* unused harmony export getEq */
/* unused harmony export getOrd */
/* unused harmony export getApplySemigroup */
/* unused harmony export getApplyMonoid */
/* unused harmony export getFirstMonoid */
/* unused harmony export getLastMonoid */
/* unused harmony export getMonoid */
/* unused harmony export option */
/* unused harmony export alt */
/* unused harmony export ap */
/* unused harmony export apFirst */
/* unused harmony export apSecond */
/* unused harmony export chain */
/* unused harmony export chainFirst */
/* unused harmony export duplicate */
/* unused harmony export extend */
/* unused harmony export filter */
/* unused harmony export filterMap */
/* unused harmony export flatten */
/* unused harmony export foldMap */
/* unused harmony export map */
/* unused harmony export partition */
/* unused harmony export partitionMap */
/* unused harmony export reduce */
/* unused harmony export reduceRight */
/* unused harmony export compact */
/* unused harmony export separate */
/* unused harmony export fromEither */
@OliverJAsh thanks for the info. I haven't tried that as the webpack config is hidden away behind a CLI (in this case Angular). If I make any progress based on your hint I'll report back.
Isn't this enough for TS?
@gkamperis I don't believe so—that only affects the behaviour at compile time. paths
does not modify module URLs at runtime.
@OliverJAsh I just tested the fp-ts
branch of your tree-shaking-test
repository. I see the same unused harmony export
messages in development mode.
Then I changed mode
to production
, generated the index.ts
and formatted it, and it looks to me like many other Option
functions are included:
https://gist.github.com/wmaurer/a1f2592a8e15b3a85c383f6077617b13#file-index-js-L144
Do you see the same thing?
Yeah, interesting. I think it's because tree shaking only works on exports, but option
is a regular object:
https://github.com/gcanti/fp-ts/blob/b1102b706be3dc023bfc968e8a4e03d6f0a2e607/src/Option.ts#L622
That does seem a shame. @gcanti Do you have any thoughts on this?
I think option
needs all these methods, otherwise things like sequenceT
won't work:
https://github.com/gcanti/fp-ts/blob/60719ba83232660612208c4daaa61f8034470a5e/src/Apply.ts#L91
At least I know now I'm not being shackled by my webpack config being hidden behind a CLI. Importing using the fp-ts/es6/Option
syntax already helps a lot with tree-shaking.
In theory, that could still work if option
was constructed as a namespace (* as option
) instead of as an object—then tree shaking would be able to work.
Just thought I'd link issues (#1053) and mention that the the solution from @OliverJAsh works well enough for me, but it's webpack specific. Other users using rollup need another solution.
@wmaurer there are two distinct issues / steps we can work on
es6
folder (the current situation) lib
or es6
in code (breaking change?)for what concerns the first step, I'd fix the wrong imports by rewriting them:
monocle-ts
: https://github.com/gcanti/monocle-ts/commit/8be0cf8385759128a0c68b319c1407d4d468e9e3🔥
@gcanti As a potential further step, I wonder what we could do about tree shaking objects like option
(mentioned above)? Namespace imports might be able to help here 🤔
@gcanti I'm in no way an expert in these packaging problems, but I have a feeling that it should be possible to address both issues with an 'optimised' package build. With a bit of searching I found this: https://github.com/pikapkg/pack What do you think?
EDIT: I'll try to find some time over the festive days to look into this ...
@OliverJAsh not sure what we can do, what are you suggesting?
@wmaurer thanks, I'll take a look. In the meanwhile though I'd like to fix the current situation, even with a temporary solution: AFAIK the es6
folder, in projects like monocle-ts
or io-ts
, looks useless at the moment
- a way to avoid having to specifying either
lib
ores6
in code (breaking change?)
This could be done without a breaking change, but in the long run we should move from fp-ts/lib/X
and fp-ts/es6/X
to fp-ts/X
. This will be a breaking change, but it can be done gracefully over multiple major versions, giving users enough time to migrate their codebase (should be a simple search&replace).
@gcanti Do you plan to merge/remove lib and es6 for version 3?
Btw, what do you think about this: import { Option, TaskEither } from 'fp-ts'
Maybe TypeScript 3.8 export * as ns Syntax
will help.
Do you plan to merge/remove lib and es6 for version 3?
@steida I agree with @wereHamster
in the long run we should move from fp-ts/lib/X and fp-ts/es6/X to fp-ts/X
When tree shaking works, could there also be a single-letter import (as most devs use it)?
So instead of:
import * as O from 'fp-ts/Option';
import * as E from 'fp-ts/Either';
One could write:
import { O, E } from 'fp-ts'
I know it's not explicit, but it's a simple explanation in docs away and it would remove those long imports at the beginning.
@gcanti Is there any technical reason I am not aware of why the whole API can't be flat and point-free?
pipe(
lastArray(context),
mapOption(entry => entry.type),
chainOption(type =>
(type as any)._tag === 'RefinementType'
? someOption(type.name)
: noneOption,
),
);
Btw, the current recommend consuming is tricky, because of clashes between Option and Either chain for example.
@robinpokorny
import { O, E } from 'fp-ts'
This is already possibly actually 😄 I'm using this:
import { option, either } from 'fp-ts'
Moreover IDE autoimport works smoothly
@raveclassic, I see, that is not documented, right? 😄 However, I was looking for something like this: https://github.com/gcanti/fp-ts/issues/952
@robinpokorny
that is not documented, right? 😄
That's just an index.ts
main entry with reexports from submodules :)
Also I think this module could easily serve as prelude as well.
I suppose the whole fp-ts
ecosystem namespaces should be somehow consolidated. This is an example from a simple sign up form:
// fp stuff
import * as E from 'fp-ts/lib/Either';
import { constVoid } from 'fp-ts/lib/function';
import * as O from 'fp-ts/lib/Option';
import { pipe } from 'fp-ts/lib/pipeable';
import * as t from 'io-ts';
import { option } from 'io-ts-types/lib/option';
// my app
import { NextPage } from 'next';
import React from 'react';
import { View, ScrollView } from 'react-native';
So many imports for "Hello world." Tree shaking is what we should optimize for, so I recommend to merge fp-ts, io-ts, io-ts-types, monocle-ts, newtype-ts libraries into just one mono library - fp-ts
ftw.
Is it possible? Or am I terribly wrong?
@steida Do you mean monorepo?
@raveclassic Why it should be necessary? Mono "lib" should be enough.
for what concerns the first step, I'd fix the wrong imports by rewriting them:
@gcanti Do you have any updates on this? I understand you made the change to some libraries in the ecosystem—which ones are remaining, if any?
If this is done, I think we can disregard the following part from my original post:
Caveat: libraries such as io-ts import from the
lib
folder, not thees6
folder. This means that tree shaking will not be able to work, for example when using the following import:
@OliverJAsh I've "fixed" all libraries (except for elm-ts
)
I have manually retested the behavior with Next.js, and:
1) import { option } from 'fp-ts';
is not tree shakeable.
2) The recommended pattern
import * as E from 'fp-ts/lib/Either';
import { pipe } from 'fp-ts/lib/pipeable';
is tree-shaked and we don't need Next.js Webpack rewriting.
@gcanti I believe we can remove /lib and /es6 path. Am I correct?
Also, why do we need * as Foo
pattern?
@steida are you sure import … from "fp-ts"
is not tree shakeable? From a purely semantic point of view, that is what you should use in modern code, and indeed rollup manages to tree shake everything that is not used. Though some of the constructs used inside the fp-ts source code make tree shaking impossible, but there is nothing bundlers can do about that.
For example, consider these lines: https://github.com/gcanti/fp-ts/blob/12c5f51e551b4b1a382e936565ebf0b8f1338d9c/src/Option.ts#L696-L717. The function call to pipeable()
as well as anything it returns will remain in the bundle because it is considered a side effect…
It also seems that webpack has some troubles tree shaking the re-exports in src/index.ts. For example when I import { option } from 'fp-ts'
but then only use a single function, webpack will retain all of the exports from the Option module. Rollup (at least version 2 which I tested), will tree shake everything but that single function. I wouldn't necessarily call it a bug in webpack, but a deficiency that will hopefully be rectified at some point.
I tried it with Next.js which uses Webpack. Maybe rollup works, but who uses rollup for app development? 🤷♀️
FWIW, webpack 5 does have a much improved tree-shaking, it appears to be on par with rollup. I just tried to bundle my fp-ts example project with webpack 5 and tree shaking works to the extent possible.
Damn. Next.js uses Webpack 4. Thank you for your insight!
OK, IDE auto-import DX is so good, that I am going to use
import { either, pipeable, option } from 'fp-ts';
pattern everywhere. Next.js will use Webpack 5 soon anyway.
@wereHamster, @gcanti Just out of a curiosity, would not be the ideal solution fully-qualified naming like optionFromEither, mapOption, chainTaskEither, .etc? Is there any reason I'm not aware of why fp-ts can't be just one flat module with re-exports?
@steida that's more a question of ergonomics. That layout is not better or worse for bundlers wrt. tree shaking.
On my Next.js 10 app, the import { … } from 'fp-ts';
pattern increases my bundle size by 10kb. The import * as x from 'fp-ts/xxx'
pattern shaves off those extra bytes.
@bstro Add this to package.json
"resolutions": {
"webpack": "^5.0.0"
},
There are a few gotchas around tree shaking, so it would be useful to document these somewhere. This applies to other libraries in the fp-ts ecosystem, such as monocle-ts.
Below is my understanding. Is this correct? If so, perhaps we could add this to the docs.
Tree shaking works out of the box, automatically thanks to the
module
field inpackage.json
.This will end up importing
fp-ts/es6/index.js
(rather thanfp-ts/lib/index.js
).Caveat: when importing sub modules however, you must remember to import from the
es6
folder:Caveat: libraries such as io-ts import from the
lib
folder, not thees6
folder. This means that tree shaking will not be able to work, for example when using the following import:The solution is to use webpack's
resolve.alias
configuration:In the case of using other libraries such as fp-ts-rxjs, it may also be necessary to create aliases for them too.