You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2022/01/04 09:03:45 UTC
[GitHub] [echarts] pissang opened a new pull request #16318: using utility methods for value type checking
pissang opened a new pull request #16318:
URL: https://github.com/apache/echarts/pull/16318
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [ ] bug fixing
- [ ] new feature
- [x] others
### What does this PR do?
This PR replaces all `typeof xxx === 'string'`, ``typeof xxx === 'number'``, ``typeof xxx === 'function'`` to `isString`, `isNumber`, `isFunction` utility methods to have neater codes. It reduces 1565 bytes in total after minimized.
And the related benchmark about this change: https://jsbench.me/yskxzuqb9t/1 . Changing to function call wont having performance drop. The interesting thing is some optimizations we did before like caching the typeof result in https://github.com/apache/echarts/blob/master/src/data/helper/dataValueHelper.ts#L158 will lead to worse performance.
This PR also removes circular dependency introduced in https://github.com/apache/echarts/pull/16300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang merged pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #16318:
URL: https://github.com/apache/echarts/pull/16318
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r777953575
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
Removed this export.
> I ever thought if it may be slightly better to extract all the mathematic functions to a common helper
I'm not sure about it. It will be helpful if some mathematic functions needs to be polyfilled(e.g. Math.hypot) or implentated. But If we only export the methods that exits in the `Math` ns. It seems to be too tricky to reduce the code size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r777953575
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
Removed this export.
> I ever thought if it may be slightly better to extract all the mathematic functions to a common helper
I'm not sure about it. It will be helpful if some mathematic functions needs to be polyfilled(e.g. Math.hypot) or implentated. But If we only export the methods that exits in the `Math` module. It seems to be too tricky to reduce the code size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#issuecomment-1004633386
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
The pull request is marked to be `PR: author is committer` because you are a committer of this project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r777950385
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -49,7 +49,8 @@ import OrdinalScale from '../../scale/Ordinal';
import { isCartesian2DSeries, findAxisModels } from './cartesianAxisHelper';
import { CategoryAxisBaseOption, NumericAxisBaseOptionCommon } from '../axisCommonTypes';
import { AxisBaseModel } from '../AxisBaseModel';
-import { alignScaleTicks, isIntervalOrLogScale } from '../../scale/helper';
+import { isIntervalOrLogScale } from '../../scale/helper';
+import { alignScaleTicks } from "../axisAlignTicks";
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r780960046
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
It should reduce some redundant `Math`, `degree to radian`, and other commonly used values like `2π, 1/2π` to shrink the bundle size. But this may be insignificant.
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
@pissang It should reduce some redundant `Math`, `degree to radian`, and other commonly used values like `2π, 1/2π` to shrink the bundle size. But this may be insignificant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r780912463
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
@plainheart Seems d3 did the similar thing:
https://github.com/d3/d3-geo/blob/main/src/math.js#L21
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
@plainheart Seems d3 does the similar thing:
https://github.com/d3/d3-geo/blob/main/src/math.js#L21
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r777923648
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
I ever thought if it may be slightly better to extract all the mathematic functions to a common helper. Any idea? If it's feasible, that'll be a huge job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r777950437
##########
File path: src/coord/radar/Radar.ts
##########
@@ -28,8 +28,8 @@ import GlobalModel from '../../model/Global';
import ExtensionAPI from '../../core/ExtensionAPI';
import { ScaleDataValue } from '../../util/types';
import { ParsedModelFinder } from '../../util/model';
-import { map, each } from 'zrender/src/core/util';
-import { alignScaleTicks } from '../../scale/helper';
+import { map, each, isString, isNumber } from 'zrender/src/core/util';
+import { alignScaleTicks } from "../axisAlignTicks";
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#discussion_r777924878
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -49,7 +49,8 @@ import OrdinalScale from '../../scale/Ordinal';
import { isCartesian2DSeries, findAxisModels } from './cartesianAxisHelper';
import { CategoryAxisBaseOption, NumericAxisBaseOptionCommon } from '../axisCommonTypes';
import { AxisBaseModel } from '../AxisBaseModel';
-import { alignScaleTicks, isIntervalOrLogScale } from '../../scale/helper';
+import { isIntervalOrLogScale } from '../../scale/helper';
+import { alignScaleTicks } from "../axisAlignTicks";
Review comment:
Unexpected double quotation mark.
##########
File path: src/coord/radar/Radar.ts
##########
@@ -28,8 +28,8 @@ import GlobalModel from '../../model/Global';
import ExtensionAPI from '../../core/ExtensionAPI';
import { ScaleDataValue } from '../../util/types';
import { ParsedModelFinder } from '../../util/model';
-import { map, each } from 'zrender/src/core/util';
-import { alignScaleTicks } from '../../scale/helper';
+import { map, each, isString, isNumber } from 'zrender/src/core/util';
+import { alignScaleTicks } from "../axisAlignTicks";
Review comment:
Unexpected double quotation mark.
##########
File path: src/scale/helper.ts
##########
@@ -17,24 +17,20 @@
* under the License.
*/
-import { NumericAxisBaseOptionCommon } from '../coord/axisCommonTypes';
-import {getPrecisionSafe, getPrecision, round, nice, quantityExponent} from '../util/number';
+import {getPrecision, round, nice, quantityExponent} from '../util/number';
import IntervalScale from './Interval';
-import { getScaleExtent } from '../coord/axisHelper';
-import { AxisBaseModel } from '../coord/AxisBaseModel';
import LogScale from './Log';
import Scale from './Scale';
-import { warn } from '../util/log';
-const mathLog = Math.log;
+export const mathLog = Math.log;
Review comment:
I ever thought if it may be slightly better to move all the mathematic functions to a common helper. Any idea? If it's feasible, that'll be a huge job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #16318: using utility methods for value type checking
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #16318:
URL: https://github.com/apache/echarts/pull/16318#issuecomment-1004693696
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org