-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Suggestion: documentation for tree shaking #1044
Comments
@OliverJAsh thanks for raising this. In your experience, does tree shaking work correctly when we use the I just tried:
and compared with importing just 5 individual functions |
@wmaurer Have you enabled 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. |
@OliverJAsh I just tested the |
I think Line 91 in 60719ba
At least I know now I'm not being shackled by my webpack config being hidden behind a CLI. Importing using the |
In theory, that could still work if |
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
for what concerns the first step, I'd fix the wrong imports by rewriting them:
|
🔥 @gcanti As a potential further step, I wonder what we could do about tree shaking objects like |
@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. 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 |
This could be done without a breaking change, but in the long run we should move from |
@gcanti Do you plan to merge/remove lib and es6 for version 3? Btw, what do you think about this: |
@steida I agree with @wereHamster
|
When tree shaking works, could there also be a single-letter import (as most devs use it)? So instead of:
One could write:
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. |
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: #952 |
That's just an |
I suppose the whole // 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 - Is it possible? Or am I terribly wrong? |
@raveclassic Why it should be necessary? Mono "lib" should be enough. |
Related: #1087 |
@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:
|
@OliverJAsh I've "fixed" all libraries (except for |
I have manually retested the behavior with Next.js, and:
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 |
@steida are you sure For example, consider these lines: Lines 696 to 717 in 12c5f51
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 |
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. |
@bstro Add this to
|
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.
The text was updated successfully, but these errors were encountered: