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

fix: Uncaught TypeError: Failed to execute 'createRadialGradient' on … #898

Merged
merged 8 commits into from
Apr 15, 2022

Conversation

lefex
Copy link
Contributor

@lefex lefex commented Mar 9, 2022

…'CanvasRenderingContext2D': The provided double value is non-finite.

at createRadialGradient. Excute npm run test error fix! (#16649)

The bug in Echarts apache/echarts#16649

…'CanvasRenderingContext2D': The provided double value is non-finite.

    at createRadialGradient. Excute npm run test error fix! (#16649)
…'CanvasRenderingContext2D': The provided double value is non-finite.

    at createRadialGradient. Excute npm run test error fix! (#16649)
…'CanvasRenderingContext2D': The provided double value is non-finite.

    at createRadialGradient. Excute npm run test error fix! (#16649)
…'CanvasRenderingContext2D': The provided double value is non-finite.

    at createRadialGradient. Excute npm run test error fix! (#16649)
@@ -4,30 +4,36 @@ import { GradientObject } from '../graphic/Gradient';
import { RectLike } from '../core/BoundingRect';
import Path from '../graphic/Path';

export function safeNum(num: number, defalutNum: number = 0, negative = false) {
// null NaN or empty string
if (num === null || isNaN(num) || `${num}` === '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems isNaN(num) also includes null and '' check.

Copy link
Contributor Author

@lefex lefex Mar 10, 2022

Choose a reason for hiding this comment

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

the last submit have removed

@@ -4,30 +4,41 @@ import { GradientObject } from '../graphic/Gradient';
import { RectLike } from '../core/BoundingRect';
import Path from '../graphic/Path';

export function safeNum(num: number, defalutNum: number = 0, negative = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use name nonNegative will be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have changed

@plainheart
Copy link
Collaborator

If we filter these illegal values in the underlying zrender without any warnings, will it hide potential errors of arguments passed from the upper application like ECharts? Most of the time, I think such an issue is caused by error arguments from the callers.

…'CanvasRenderingContext2D': The provided double value is non-finite.

    at createRadialGradient. Excute npm run test error fix! (#16649)
@lefex
Copy link
Contributor Author

lefex commented Mar 10, 2022

If we filter these illegal values in the underlying zrender without any warnings, will it hide potential errors of arguments passed from the upper application like ECharts? Most of the time, I think such an issue is caused by error arguments from the callers.

I think we should check the parameters to make sure there are no errors.

@plainheart
Copy link
Collaborator

It may be more difficult for the developers to find out why they didn't get the expected result as no errors were thrown and no warnings were output.

@pissang
Copy link
Contributor

pissang commented Mar 10, 2022

Usually we prefer warning logs over exceptions when having invalid values.

@Ovilia
Copy link
Member

Ovilia commented Mar 11, 2022

How about giving warning as well as doing such safe guarding?
You can use warn function in util/log to do so.

@lefex
Copy link
Contributor Author

lefex commented Mar 11, 2022

How about giving warning as well as doing such safe guarding? You can use warn function in util/log to do so.

ok!

…'CanvasRenderingContext2D': The provided double value is non-finite.

    at createRadialGradient. Excute npm run test error fix! (#16649)
@lefex
Copy link
Contributor Author

lefex commented Mar 11, 2022

I add a warning in this pr . I'm not going to change the createLinearGradient method.

@pissang @Ovilia

return false;
}
// NaN, Infinity, undefined, 'xx'
if (!isFinite(num)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!isFinite also includes num === null check.

These three check can be merged to:

if (!isFinite(num) || (nonNegative && num < 0 {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry it's my mistake. null can be convert to 0 here. I rechecked if null can be used as argument of createLinearGradient and seems it can. But undefined will throw an non-finite exception. So if we mainly wan using this check to avoid exceptions, we can still remove null check.

if (!obj.global) {
x = x * width + rect.x;
y = y * height + rect.y;
r = r * min;
}

if (!isSafeNum(x) || !isSafeNum(y) || !isSafeNum(r, true)) {
console.warn('The provided value is non-finite. You must provide x and y.');
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

return nothing here is not safe. Because we expect this method to return a CanvasGradient and set it to CanvasRenderingContext. If we return null, it will be an undefined behavior.

I think we can fallback to default value when the x, y, r is invalid like createLinearGradient did.

Also there are several concerns about this warn message being to frequently:

  1. It should be warn only once if the CanvasGradientObject is not changed. Use WeakMap to mark this CanvasGradientObject has been warned may be a solution.
  2. The boundingRect sometimes may be infinite or NaN(when we don't want to show the element) and leading this x, y, r to be invalid. In this case, the error message will not be helpful but confuse the
    developers. So it's better to check the obj.x, obj.y, obj.r earlier and give warning message. Then we do check x, y, r again and fallback to default value.
  3. The check of obj.x, obj.y, obj.r and warning message can be done only when process.env.NODE_ENV !== 'production' so these can be ommitted in the production environment.

At last the createLinearGradient should do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The obj is changed when mouse hover, can not use WeakMap to add warning log only once. So I added a guard to ensure that the parameters are valid.

image

…'CanvasRenderingContext2D': The provided double value is non-finite.

    at createRadialGradient. Excute npm run test error fix! (#16649)
if (!obj.global) {
x = x * width + rect.x;
y = y * height + rect.y;
r = r * min;
}

x = isSafeNum(x) ? x : 0.5;
y = isSafeNum(y) ? y : 0.5;
r = isSafeNum(r, true) ? r : 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems only one number needs to be check positive. In this case I will prefer keeping the isSafeNum simpler and more cohesion that only accept one parameter number. Monomorphic function is always better than polymorphic function, on both code style and performance.

So the r check can be

r = isSafeNum(r) && r >= 0 ? r : 0.5

@@ -12,7 +12,7 @@ describe('Path', function () {
cy: 625.5,
startAngle: -1.5707963267948966,
endAngle: 4.71238898038469,
innerCornerRadius: 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems to be unnecessary?

@pissang pissang merged commit ddc7e8b into ecomfe:master Apr 15, 2022
Ovilia added a commit that referenced this pull request Jun 7, 2022
fix(gradient): #898 has bug about safe checkingand breaks all gradients
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants