acdlite / recompose

A React utility belt for function components and higher-order components.
MIT License
14.75k stars 1.26k forks source link

Deprecate direct imports of enhancers #389

Open istarkov opened 7 years ago

istarkov commented 7 years ago

For now we have an ability to import every enhancer directly like import withProps from 'recompose/withProps', there are 2 reasons to deprecate such behaviour: 1) Now we are supporting es2015 modules, so direct import is not the best way to reduce size 2) Two different imports can be a problem, as an example current problem

import { setObservableConfig } from 'recompose';
import createEventHandler from 'recompose/createEventHandler';
import rxjsObservableConfig from 'recompose/rxjsObservableConfig';
setObservableConfig(rxjsObservableConfig);
const { handler, stream } = createEventHandler();
// OOPS stream is not rxjs stream because `createEventHandler` imported directly and use other instance of _config object

We can somehow add a deprecation warning at a build step for every enhancer, (not stream configs), or just remove them and release a new major version.

TrySound commented 7 years ago

@istarkov I guess this case is for major. It's not kind of API.

denisborovikov commented 7 years ago

Hey, guys!

Here's a jscodeshift mod to automatically convert imports in your project as proposed by @istarkov.

cd /path/to/your/project

# Install jscodeshift (if you have not already)
yarn add jscodeshift

# Checkout the mod 
git clone git@gist.github.com:0c38799b83d75e2aef529a3560eb65d7.git codemods

# Run migration, replace ./src with your project files root
jscodeshift -t ./codemods/convert-recompose-imports.js ./src

Please note, if you have a comment right before recompose's import, it will be lost.

/* eslint-disable react/no-did-mount-set-state */
import lifecycle from 'recompose/lifecycle';
wuct commented 7 years ago

We should add a warning in next patch version (might be v0.23.5) first.

threehams commented 7 years ago

What's the suggested way to reduce size? Does this assume tree shaking is really working reliably in apps, including ones using Typescript? Without it working reliably, this would be a huge performance regression - direct imports are still the only way I know to reliably avoid bundling unused code.

Based on these open PRs in Webpack (and my own experience of having no luck getting it working with components or HOCs), tree shaking has a long way to go: https://github.com/webpack/webpack/issues/1750 https://github.com/webpack/webpack/issues/2867

TrySound commented 7 years ago

We have tests for them. All is good. Your components may not treeshake if you use Babel display name plugin because it's expressions in module scope.

threehams commented 7 years ago

That tree shaking test suite is fantastic, and I'm absolutely going to lift that idea for another project. Thanks for commenting!

There's still some overhead (603 bytes gzipped or so), but I see there are outstanding issues for that. Rollup really does a very nice job of getting around the Uglify side effect complaints.