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.

Sign up for GitHub

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

refactor: moved to compatible with React 17 #48

Merged
merged 3 commits into from
Mar 21, 2021
Merged

refactor: moved to compatible with React 17 #48

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c6f75f
refactor: moved to compatible with React 17
Mar 8, 2021
0cbd489
refactor: removed lodash, decreased size by 55%
Mar 8, 2021
87fffc1
ci: added Github Action ci
Mar 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
},
"parser": "babel-eslint",
"rules": {
"no-restricted-globals": [1, "isFinite"],
"valid-jsdoc": 2,
"react/jsx-uses-react": 2,
"react/jsx-uses-vars": 2,
Expand Down
1 change: 0 additions & 1 deletion babel.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const BABEL_ENV = process.env.BABEL_ENV;

const plugins = [
'lodash',
'@babel/plugin-proposal-export-default-from',
'@babel/plugin-proposal-export-namespace-from',
['@babel/plugin-proposal-decorators', { legacy: true }],
Expand Down
33 changes: 5 additions & 28 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@
"homepage": "https://github.com/recharts/react-smooth#readme",
"peerDependencies": {
"react": "^15.0.0 || ^16.0.0 || ^17.0.0",
"react-dom": "^15.0.0 || ^16.0.0 || ^17.0.0"
"react-dom": "^15.0.0 || ^16.0.0 || ^17.0.0",
"prop-types": "^15.6.0"
},
"dependencies": {
"lodash": "^4.17.4",
"prop-types": "^15.6.0",
"fast-equals": "^2.0.0",
"raf": "^3.4.0",
"react-transition-group": "2.9.0"
},
Expand All @@ -68,7 +68,6 @@
"@testing-library/react": "^11.2.5",
"babel-eslint": "^10.0.0",
"babel-loader": "^8.0.0",
"babel-plugin-lodash": "^3.3.0",
"core-js": "^3.9.1",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's unused? Corejs is required by presetenv

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gather it has to be explicitly enabled, and is disabled by default:

https://babeljs.io/docs/en/babel-preset-env#usebuiltins

(Which is good. Polyfill = application responsibility, not library)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not 100% right, on recharts we've been shipping library compiled with env to be compatible with at least ie11 and last 2 versions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed, and I think it's a bit unusual. The use of Webpack itself here is a bit unusual. It's a tool for bundling client js for applications. The underlying babel can/should be used by itself when we are creating libraries. Under the covers, webpack is just calling upon babel to do the transpile.

axios is another famous project for promulgating this approach, which has sharp edges.

How do large projects end up committing seemingly obvious errors?

Consider the timeline: in 2016 and earlier, we had very limited choices for putting together demos/docs. Naturally, webpack can do this for us, but some project(s) end up using webpack for the build too, which is really better left for babel alone to do.

Seen from an application developers' perspective: "Oh, guess I picked up another Promise implementation." or "Here goes another way to do Object.assign." The developer now needs to a) address bloat if there is a lot of polyfill cruft, b) worry about conflicts of interest in competing polyfill solutions.

The application developer needs to be in charge of their target environment(s). If they need a polyfill, they should add it... we cannot possibly imagine all the possible environments the code could be running in. And it's simple enough for the consumer to just add what they need.

Copy link

@avindra avindra Mar 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get back on topic: @semoal

For this PR: I think it's fine if you need to add core-js (it was in recharts main repo at some point), but prefer to leave it out if possible. The creator is finally out of jail, which is good I guess.

I will write a more detailed post about this problem soon, as I haven't seen this problem described in any detail on the web. Sorry for the rant.

"cross-env": "^5.0.5",
"eslint": "^4.8.0",
Expand Down
4 changes: 2 additions & 2 deletions src/Animate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { PureComponent, cloneElement, Children } from 'react';
import PropTypes from 'prop-types';
import _ from 'lodash';
import { deepEqual } from 'fast-equals';
import createAnimateManager from './AnimateManager';
import { configEasing } from './easing';
import configUpdate from './configUpdate';
Expand Down Expand Up @@ -128,7 +128,7 @@ class Animate extends PureComponent {
return;
}

if (_.isEqual(prevProps.to, this.props.to) && prevProps.canBegin && prevProps.isActive) {
if (deepEqual(prevProps.to, this.props.to) && prevProps.canBegin && prevProps.isActive) {
return;
}

Expand Down
10 changes: 7 additions & 3 deletions src/AnimateGroupChild.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
import React, { Component, Children } from 'react';
import { Transition } from 'react-transition-group';
import PropTypes from 'prop-types';
import _ from 'lodash';
import Animate from './Animate';

if (Number.isFinite === undefined) {
Number.isFinite = function (value) {
return typeof value === 'number' && isFinite(value);
};
}

const parseDurationOfSingleTransition = (options = {}) => {
const { steps, duration } = options;

if (steps && steps.length) {
return steps.reduce((result, entry) => (
result + (_.isNumber(entry.duration) && entry.duration > 0 ? entry.duration : 0)
result + (Number.isFinite(entry.duration) && entry.duration > 0 ? entry.duration : 0)
), 0);
}

if (_.isNumber(duration)) {
if (Number.isFinite(duration)) {
return duration;
}

Expand Down
4 changes: 2 additions & 2 deletions src/configUpdate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import raf, { cancel as caf } from 'raf';
import { filter } from 'lodash';
import {
getIntersectionKeys,
mapObject,
Expand Down Expand Up @@ -65,7 +64,8 @@ export default (from, to, easing, duration, render) => {
let update = () => null;

const getCurrStyle = () => mapObject((key, val) => val.from, stepperStyle);
const shouldStopAnimation = () => !filter(stepperStyle, needContinue).length;
console.log(needContinue);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

const shouldStopAnimation = () => !Object.values(stepperStyle).filter(needContinue).length;

// stepper timing function like spring
const stepperUpdate = (now) => {
Expand Down
7 changes: 4 additions & 3 deletions src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { intersection } from 'lodash';

/* eslint no-console: 0 */
const PREFIX_LIST = ['Webkit', 'Moz', 'O', 'ms'];
const IN_LINE_PREFIX_LIST = ['-webkit-', '-moz-', '-o-', '-ms-'];
const IN_COMPATIBLE_PROPERTY = ['transform', 'transformOrigin', 'transition'];

export const getIntersectionKeys = (preObj, nextObj) =>
intersection(Object.keys(preObj), Object.keys(nextObj));
[
Object.keys(preObj),
Object.keys(nextObj),
].reduce((a, b) => a.filter(c => b.includes(c)));

export const identity = param => param;

Expand Down
6 changes: 6 additions & 0 deletions webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ const config = {
commonjs: 'react-transition-group',
amd: 'react-transition-group',
},
'prop-types': {
root: 'PropTypes',
commonjs2: 'prop-types',
commonjs: 'prop-types',
amd: 'prop-types',
},
},

plugins: [
Expand Down