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

Suggestion: documentation for tree shaking #1044

Open
OliverJAsh opened this issue Dec 12, 2019 · 37 comments
Open

Suggestion: documentation for tree shaking #1044

OliverJAsh opened this issue Dec 12, 2019 · 37 comments

Comments

@OliverJAsh
Copy link
Collaborator

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 in package.json.

import { option } from 'fp-ts';

This will end up importing fp-ts/es6/index.js (rather than fp-ts/lib/index.js).

Caveat: when importing sub modules however, you must remember to import from the es6 folder:

-import * as O from 'fp-ts/lib/Option';
+import * as O from 'fp-ts/es6/Option';

Caveat: libraries such as io-ts import from the lib folder, not the es6 folder. This means that tree shaking will not be able to work, for example when using the following import:

import * as t from 'io-ts';

The solution is to use webpack's resolve.alias configuration:

module.exports = {
  //...
  resolve: {
    alias: {
      'fp-ts/lib': 'fp-ts/es6'
    }
  }
};

In the case of using other libraries such as fp-ts-rxjs, it may also be necessary to create aliases for them too.

@wmaurer
Copy link
Contributor

wmaurer commented Dec 12, 2019

@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.

@OliverJAsh
Copy link
Collaborator Author

OliverJAsh commented Dec 12, 2019

@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 */

@wmaurer
Copy link
Contributor

wmaurer commented Dec 12, 2019

@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.

@gkamperis
Copy link

Isn't this enough for TS?

Tsc crash when mixing fp-ts/lib & fp-ts/es6 #860

@OliverJAsh
Copy link
Collaborator Author

OliverJAsh commented Dec 16, 2019

@gkamperis I don't believe so—that only affects the behaviour at compile time. paths does not modify module URLs at runtime.

@wmaurer
Copy link
Contributor

wmaurer commented Dec 17, 2019

@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?

@OliverJAsh
Copy link
Collaborator Author

Yeah, interesting. I think it's because tree shaking only works on exports, but option is a regular object:

export const option: Monad1<URI> &

That does seem a shame. @gcanti Do you have any thoughts on this?

@wmaurer
Copy link
Contributor

wmaurer commented Dec 17, 2019

I think option needs all these methods, otherwise things like sequenceT won't work:

* const sequenceTOption = sequenceT(option)

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.

@OliverJAsh
Copy link
Collaborator Author

OliverJAsh commented Dec 17, 2019

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.

@wmaurer
Copy link
Contributor

wmaurer commented Dec 18, 2019

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.

@gcanti
Copy link
Owner

gcanti commented Dec 18, 2019

@wmaurer there are two distinct issues / steps we can work on

  • wrong imports in the es6 folder (the current situation)
  • a way to avoid having to specifying either lib or es6 in code (breaking change?)

for what concerns the first step, I'd fix the wrong imports by rewriting them:

@OliverJAsh
Copy link
Collaborator Author

OliverJAsh commented Dec 18, 2019

🔥

@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 🤔

@wmaurer
Copy link
Contributor

wmaurer commented Dec 19, 2019

@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 ...

@gcanti
Copy link
Owner

gcanti commented Dec 20, 2019

@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

@wereHamster
Copy link

wereHamster commented Dec 22, 2019

  • a way to avoid having to specifying either lib or es6 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).

@steida
Copy link
Contributor

steida commented Jan 12, 2020

@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.

@gcanti
Copy link
Owner

gcanti commented Jan 12, 2020

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

@robinpokorny
Copy link
Sponsor

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.

@steida
Copy link
Contributor

steida commented Jan 13, 2020

@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.

@raveclassic
Copy link
Collaborator

@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

@robinpokorny
Copy link
Sponsor

@raveclassic, I see, that is not documented, right? 😄 However, I was looking for something like this: #952

@raveclassic
Copy link
Collaborator

@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.

@steida
Copy link
Contributor

steida commented Jan 14, 2020

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?

@raveclassic
Copy link
Collaborator

@steida Do you mean monorepo?

@steida
Copy link
Contributor

steida commented Jan 14, 2020

@raveclassic Why it should be necessary? Mono "lib" should be enough.

@OliverJAsh
Copy link
Collaborator Author

Related: #1087

@OliverJAsh
Copy link
Collaborator Author

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 the es6 folder. This means that tree shaking will not be able to work, for example when using the following import:

@gcanti
Copy link
Owner

gcanti commented Feb 13, 2020

@OliverJAsh I've "fixed" all libraries (except for elm-ts)

@steida
Copy link
Contributor

steida commented Apr 10, 2020

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?

@wereHamster
Copy link

@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:

fp-ts/src/Option.ts

Lines 696 to 717 in 12c5f51

const {
alt,
ap,
apFirst,
apSecond,
chain,
chainFirst,
duplicate,
extend,
filter,
filterMap,
flatten,
foldMap,
map,
partition,
partitionMap,
reduce,
reduceRight,
compact,
separate,
fromEither
} = pipeable(option)
. 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.

@steida
Copy link
Contributor

steida commented Apr 10, 2020

I tried it with Next.js which uses Webpack. Maybe rollup works, but who uses rollup for app development? 🤷‍♀️

@wereHamster
Copy link

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.

@steida
Copy link
Contributor

steida commented Apr 10, 2020

Damn. Next.js uses Webpack 4. Thank you for your insight!

@steida
Copy link
Contributor

steida commented Apr 18, 2020

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?

@wereHamster
Copy link

@steida that's more a question of ergonomics. That layout is not better or worse for bundlers wrt. tree shaking.

@bstro
Copy link

bstro commented Feb 2, 2021

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.
image
image

@steida
Copy link
Contributor

steida commented Feb 2, 2021

@bstro Add this to package.json

"resolutions": {
  "webpack": "^5.0.0"
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants