Do we need the else here ?
I don't think the branches are exclusive.

Also maybe move process.emitWarning(typeCastWarning); right after l. 594 as soon as we know that the type is a number ?

They are exclusive. They both recommend the same outcome, but I think having two separate warnings is a bit confusing. This one warning is it could be dangerous, the other is destructive. I had what you recommended earlier but it is very very noisy :)

Note: added 'debouncing' you will only get a single typecast warning for the entity. that way it doesn't flood the warning feed.

It might be reasonable to augment the out of bounds warning to mention the type issue as well, since that'll still be an issue if they fix their number size issue.

Since we're debouncing the typescast warning any ways, I'd consider just dropping the if and printing both.

I think printing both could get noisy as Chris said. I ended up going with @feywind's suggestion here.

Are Double / Int included in this type ?

They ought to be, via the Number type. Though that isn't really a use path. Support for that could be added, but as you might detect a trend, this is a mirroring of the existing style we went with for Ints.

They ought to be, via the Number type

There are not because number vs Number

see https://www.typescriptlang.org/play?#code/MYGwhgzhAECyCeA5aBTAHgFxQOwCY0QFcBbAIxQCdoBvAXwCh6AzQ7YDASwHttomwOIABTYAXNGwlyFAJQ160RdGA8IXECgB0ILgHMRMgNz0GzVu268uAaxHiiZSnOoKlK7Go3a9B46f6CIigA7nBIQjJG9DZBoQiIEVH0QA

Is there a case where we'd want to construct a Number from another Number? If not, I'd start with accepting a string, number, and potentially ValueProto (as we are).

It seems like there is never a common case where we would want to construct a Double from a Double / Int. Constructing a Double from a Double is just creating a copy and making a Double from an Int could encourage users to misuse the API by inadvertently changing the column type.

What is the use case of constructing from a proto ?
If it needs to be supported int would probably make sense too.

Int has the same support from proto, and it is part of the entityFromEntityProto path. This code mirrors the behavior of our Int class.

I was wondering the same thing, the fact that we allow this in the Int type seems like good motivation 👍 Let's just make sure we have a test for it.

I added a bunch more tests to Double here.

These tests and all other changes will be in a separate PR.

Do we really need to keep the property name ?
If yes it would be good to comment why.

Honestly, I am not sure what this is for, but I mirrored behavior in the Int implementation. Maybe @stephenplusplus recalls?

If we're not 100% what it's here for, I'd be tempted to drop it (let's chat with @stephenplusplus offline).

I haven't checked the use cases but it seems dangerous

if an entity has the form {oldValue: Int, newValue: Int} and you do (with wrapped Number):

const [entity] = await ds.get(...);

entity.oldValue = entity.newValue;
entity.newValue = ds.Int(123);

await ds.save(entity);

I can only imagine that keeping the entity property name would cause problems as oldValue would have a reference to "newValue".

I am not sure if it is actually a problem in the current code but at least it makes it easy to shoot yourself in the foot.

I'll remove this. The reason we have this for Int is because we use it when using the valueOf() function.

Why do need a value member ?
It seems like it could be dropped as well as valueOf which are both already in the super class.

Possibly? This started as a string, but realized I could store this as a number. We store it as a string for Int mostly due to Int range differences.

Datastore Doubles have 64-bit double precision, IEEE 754, as do Node.js Numbers. So I was able to change this type and simplify a bit.

feels kind of like this should be private.

I don't know about that. If we make it private then that would be a breaking change. Consider the case where users are using .value in various places for these types. In the Int class this is private.

Number is redundant with l 83.
But is seems like valueOf should be dropped altogether as you extend Number ?

Playing around with extending Number, there isn't access to a this.value unless we store the value ourselves:

class F extends Number {
  constructor(value) {
    super(value)
  }
  blerg() {
    console.info(this.value)
  }
}

const f = new F(99);
console.info(f.valueOf())
f.blerg()

outputs:

99
undefined

Part of me wonders if we should avoid extending the base Number type, and instead create a DatastoreNumber type, since Number implies a a larger interface than we potentially want to support out of the gate?

Edit: I see we're already extending Number in Int, I'd stick with this approach.

Good to know.

This seems quite dangerous if the goal of Double is to fake a Number ?

can you expand? Sorry, I don't quite understand/follow. Also, class Int has a similar toJson.

I am really not sure to get the intent of the change right but my understanding of the current state is that you want to return Ints and Doubles that users would use as if they were Numbers.

If I get this right then Ints and Doubles should behave like Numbers as much a possible.

However with this code:

• JSON.stringify(1.23) == "1.23"
• but JSON.stringify(Double(1.23)) == '{"type": ..., "value": ...}'

While not related to toJSON() returning wrapped numbers (Int / Double) could be problematic:

const [entity] = await ds.get(...);

// entity.price is a Double, i.e. Double(10)

entity.price *= 0.90;

// entity.price is now a number (9, "int")

// warning + DB screwed
await ds.save(entity);

It seems that returning wrapped numbers in Int / Double format is necessary in order to avoid the problem where we change the column type. I see how adding toJson changes the output of JSON.stringify which is technically a breaking change, but since Int currently has toJson implemented and Double / Int are likely being returned interchangably, it is worth the tradeoff to add toJSON here to ensure JSON.stringify will handle each type the same way.

They ought to be, via the Number type

There are not because number vs Number

see https://www.typescriptlang.org/play?#code/MYGwhgzhAECyCeA5aBTAHgFxQOwCY0QFcBbAIxQCdoBvAXwCh6AzQ7YDASwHttomwOIABTYAXNGwlyFAJQ160RdGA8IXECgB0ILgHMRMgNz0GzVu268uAaxHiiZSnOoKlK7Go3a9B46f6CIigA7nBIQjJG9DZBoQiIEVH0QA

I am really not sure to get the intent of the change right but my understanding of the current state is that you want to return Ints and Doubles that users would use as if they were Numbers.

If I get this right then Ints and Doubles should behave like Numbers as much a possible.

However with this code:

• JSON.stringify(1.23) == "1.23"
• but JSON.stringify(Double(1.23)) == '{"type": ..., "value": ...}'

While not related to toJSON() returning wrapped numbers (Int / Double) could be problematic:

const [entity] = await ds.get(...);

// entity.price is a Double, i.e. Double(10)

entity.price *= 0.90;

// entity.price is now a number (9, "int")

// warning + DB screwed
await ds.save(entity);

Is there a case where we'd want to construct a Number from another Number? If not, I'd start with accepting a string, number, and potentially ValueProto (as we are).

I was wondering the same thing, the fact that we allow this in the Int type seems like good motivation 👍 Let's just make sure we have a test for it.

If we're not 100% what it's here for, I'd be tempted to drop it (let's chat with @stephenplusplus offline).

Playing around with extending Number, there isn't access to a this.value unless we store the value ourselves:

class F extends Number {
  constructor(value) {
    super(value)
  }
  blerg() {
    console.info(this.value)
  }
}

const f = new F(99);
console.info(f.valueOf())
f.blerg()

outputs:

99
undefined

Part of me wonders if we should avoid extending the base Number type, and instead create a DatastoreNumber type, since Number implies a a larger interface than we potentially want to support out of the gate?

Edit: I see we're already extending Number in Int, I'd stick with this approach.

Since we're debouncing the typescast warning any ways, I'd consider just dropping the if and printing both.

Is this used anywhere, I think it might just be vscode adding something?

Not used. Will delete.

feels kind of like this should be private.

As a TODO, I wish we had unit tests for the Int and Double types, vs., only integration tests.

I think a good unit test corresponding to this one would be to mock out the get and the save and ensure that getting a Double/Int leads to saving the same type.

Not used. Will delete.

I think a good unit test corresponding to this one would be to mock out the get and the save and ensure that getting a Double/Int leads to saving the same type.