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 textbound invalid input #6302

Closed

Conversation

adityagarg06
Copy link

Resolves #6298

Changes:
Added an additional check for invalid input. Also I have written test for the same

Comment on lines +79 to +82
if (typeof str !== 'string') {
console.error('Error: Text parameter must be a string.');
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

We don't usually handle incorrect parameter types this way as the FES is usually the one to take care of it. In terms of logic it should either do type coersion or return sensible default values, null is probably not useful in this case and can cause a property access error on the user's end.

@micuat
Copy link
Member

micuat commented Jul 27, 2023

to me, coercion into string makes sense in case of a number. but then what if the input is an object, for example?

@munusshih
Copy link
Contributor

I think this should be handled in FES. Making an exception for this function might be a workaround for now, but doesn't seem to provide a general solution for this types of problem.

@Qianqianye
Copy link
Contributor

I think this should be handled in FES. Making an exception for this function might be a workaround for now, but doesn't seem to provide a general solution for this types of problem.

Thanks @munusshih. I'm wondering what the GSoC FES contributor and mentors think about this @almchung @nbriz @Ayush23Dash

@Ayush23Dash
Copy link
Contributor

+1 to what @limzykenneth said. Returning a null is not a good solution. Moreover, this might solve the issue temporarily but we need to have a general solution rather than a customised one.

@dhowe
Copy link
Contributor

dhowe commented Nov 10, 2024

status ?

@limzykenneth
Copy link
Member

@dhowe I'll close this for now as the implemented solution is not necessarily what we needed.

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