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

ReadableType does not distinguish between Integers and Doubles #4141

Closed
andpor opened this issue Nov 15, 2015 · 8 comments
Closed

ReadableType does not distinguish between Integers and Doubles #4141

andpor opened this issue Nov 15, 2015 · 8 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@andpor
Copy link

andpor commented Nov 15, 2015

This is Android specific.

While there is specific API to retrieve Int and Double form ReadableArray and ReadableMap, for some odd reason ReadableType enum does not make this separation and provides only Numeric entry. Having Numeric only is insufficient as one cannot make a decision whether to retrieve a Double or Int from the collection based on this enum value. This is needed for such applications as database interfaces where int vs double makes a huge difference.

Can this be fixed quickly?

@ide
Copy link
Contributor

ide commented Nov 15, 2015

cc @kmagiera

I do want to point out that ReadableArray is used to communicate with JS, which has only 64-bit floats. So if you need to specify that you want a Java double vs Java int, you will need to send that information separately from the numeric value.

You mentioned this is specific to Android but I imagine iOS would have a similar issue. Does iOS solve this issue in a way we could port to Android?

@andpor
Copy link
Author

andpor commented Nov 15, 2015

This cannot be sent separately. The API is SQL...it expects values...it would be ridiculous to come up with any reasonable API and force the API user to provide markers for each numeric value I think. Imagine sending a very long list of numeric values as the input...

problem is I have to bind the values to specific SQL types..so I need to know if something is a Double or and Integer...

there is getDouble and getInt...why generalize the ReadableType to Number ?

Have to check on iOS but seems not to be an issue since values are translated directly and I do not have to query for type...iOS return Objects which can be type checked by implementation after value is retrieved. In Android however, I cannot get the value without knowing the type up front - this is the issue...it would be great the have a generic get(i) or get(key) that returns an Java Object that can then be type checked...

this is a pretty significant limitation on Android...or am I missing something trivial here...?

public interface ReadableArray {

int size();
boolean isNull(int index);
boolean getBoolean(int index);
double getDouble(int index);
int getInt(int index);
String getString(int index);
ReadableArray getArray(int index);
ReadableMap getMap(int index);
ReadableType getType(int index);

Object get(int index) should return Integer or Double or Long or Boolean or String or ReadableArray or ReadableMap or NULL <---this is missing
}

@kmagiera
Copy link
Contributor

@andpor I think it's a good point.

We used to only have one number type on java side of the bridge and all JS numeric typese were mapped directly to "double". It turned out to be insufficient and we introduced integer as a separate type (with ReadableArray.getInt and ReadableMap.getInt). But since we don't use ReadableArray.getType anywhere in the codebase no one bothered to update it with newly introduced type.

We should update ReadableType now that we support two different numeric types. Required steps are:

  1. Update ReadableType.java: remove Number add Double and Integer
  2. Update implementation of ReadableArray.getType and ReadableMap.getType bothe are implemented in NDK (c++). Good place to start looking into it is
    case folly::dynamic::Type::DOUBLE:

@andpor
Copy link
Author

andpor commented Nov 16, 2015

Good to hear. The same applies to ReadableMap naturally...while I am at it...is there a reason why Long type is not in the mix with corresponding getLong ? It seems to work fine on iOS. Huge difference is SQLite as columns could be int or long...i have to cast longs to int which is obviously not ideal...you can take a look at my ios/android SQLite plugin:
https://github.com/andpor/react-native-sqlite-storage

@andpor
Copy link
Author

andpor commented Nov 19, 2015

@kmagiera - do you think this will get fixed anytime soon ?

@kmagiera
Copy link
Contributor

@andpor I'm afraid this is not sth we can fix. I thought about it a little more since I posted my answer and IMO if you don't know the context of what type to expect you should just use double.

In JS all numbers are 64bit floating point types (java double equivalent). When those numbers are passed over the bridge we serialize them to JSON. We can then read it expecting to be an integer, but the only way we can learn that the type is an integer is by parsing it. If you'd like to implement db backend and from the JS side you want to store floating point numbers in one column. It may have a terrible effect if you determine the type based on an individual value, as it may happen that this individual value that you wish to be a floating point value will be represented in a form of integer (e.g. you may be using 2.0 literal in JS, but it's going to be send as 2).

getInt have been introduced for the cases when we know that we expect integer on the native side. It provides type checking so that if you send floating-point number by mistake it would redbox. We use it for sending colors represented as 32bit ints or line number properties. IMO it's not suitable for the usecase you've mentioned and you should either make your API explicitly send an information about the type of the column, and then you getInt/getDouble appropriately or just use getDouble all the time as it would give you the exact value that you send from JS

@ide
Copy link
Contributor

ide commented Nov 20, 2015

I agree with @kmagiera since JavaScript does not distinguish between ints and doubles (aside from TypedArrays but that's unrelated...). There is only a Number type so there's no way for us to know if you meant int or double.

So if you are working with an API that distinguishes between ints and doubles, you need to be explicit about the intended type of your number. For a SQL API, one way to do this is with a prepared query format string like PostgreSQL's INSERT ($1::int, $2::double) INTO .... Then you'd parse the format string and understand whether a numeric value should be treated as an int or double.

But not sure what React Native can do for you here. Fundamentally this seems like an issue working with two different type systems (JS and Java's).

@mkonicek
Copy link
Contributor

Hi there! This issue is being closed because it has been inactive for a while.

But don't worry, it will live on with ProductPains! Check out its new home: https://productpains.com/post/react-native/readabletype-does-not-distinguish-between-integers-and-doubles

ProductPains helps the community prioritize the most important issues thanks to its voting feature.
It is easy to use - just login with GitHub.

Also, if this issue is a bug, please consider sending a PR with a fix.
We're a small team and rely on the community for bug fixes of issues that don't affect fb apps.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 20, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

5 participants