Add an IdentifiedRef.get() alias for unwrap

[stephenh]

With IdentifiedRef, I'm finding it common to do project.someRelation.unwrap() (as long as the relation is already init'd). It seems like project.someRelation.get() would be more idiomatic, and similar to project.someCollection.getItems().

Granted, there is a get(key: K) (or what not) method right now, so that name is "taken", however TypeScript does support declaring multiple type declarations for a single method, so get could return T if given no args, and T[K] if key is present.

Does that seem okay?

[B4nan]

There is load method already that does the very same thing. We could consolidate this to the get method, but personally I think it is better to have 2 methods as they are doing different things (although with same result).

I can imagine keeping unwrap and get methods while dropping load method. But as v3 is just released... :)

I am definitely keeping unwrap, as that will be the only sync method. Enhancing get method functionality is technically not a problem, I think I am fine with that.

[stephenh]

There is load method already that does the very same thing

Ah, to clarify, load is Promise<T> and I wanted a get(): T. I.e. synchronous just like unwrap().

Which yes, would mean the get(key) overload returned a promise and the get() overload would not return a promise. I understand that is not great in terms of consistency, but in the spirit of backwards compatibility/etc...

Basically I'm using unwrap now and it doesn't look terrible, but also IMO it's just not as intuitive as someCollection.getItems() is, or as someReference.get() would be (for the synchronous/not a promise behavior).

(...I'd be tempted to suggest that the current get(key): Promise method should really be load(key): Promise to get parallelism between load(key): Promise<T[key]> and load(): Promise<T> both return promises, and then get() would be freed up to be the synchronous/non-promise accessor. But, understand, v3 just released/etc.)

[B4nan]

Hmmm that does not sound like a good idea to me, maybe the method name would fit better, but having both sync/async method might cause a lot of confusion. In the whole ORM there is only one method that follows this pattern (being sync/async based on parameter), which is persist (and I am not very happy with that).

Generally speaking having multi purpose methods feels a bit like a code smell, it makes the code unnecessary complex - that is the reason why I introduced both load and unwrap methods, instead of using some bool param to handle if we wanna lazy load or not. Also here I would have to sacrifice usage of async/await, which is far away from what I want.

(...I'd be tempted to suggest that the current get(key): Promise method should really be load(key): Promise to get parallelism between load(key): Promise<T[key]> and load(): Promise both return promises, and then get() would be freed up to be the synchronous/non-promise accessor. But, understand, v3 just released/etc.)

I am not in favour of having get(key): T[k] method (regardless the name, I do not want to have sync method to access a property), the whole point of Reference wrapper was to force usage of await so we can actually lazy load things.

Also having load(key) feels a bit like it will load just the key (imagine you would have a reference under the key, like Reference<Book>.load('author')), while what it does is to load the Book object only.

Thoughts @lookfirst @pepakriz @vimmerru ?

[stephenh]

but having both sync/async method might cause a lot of confusion

Right, I very much agree, I was just offering it as a suggestion if: a) the .get(): T rename/alias of .unwrap() was decided to be sufficiently better/more ergonomic than .unwrap(): T and b) there was a hard rule against the breaking change of just removing (or renaming) the current .get(key): Promise<T[K]> method that is "in the way" of the proposed .get(). method.

I am not in favour of having get(key): T[k] method

Agreed.

the whole point of Reference wrapper was to force usage of await so we
can actually lazy load things

True. As a sort of field report, what I'm personally finding is that putting await on every single relation access is actually pretty tedious (i.e. for (const child of (await parent.children.init()).getItems()) { ... }), especially in chunks of code where I know the collection/reference is already loaded.

So my thought was that, just like Collection.getItems() is synchronous but still runtime-checked (i.e. will fail if its not loaded), having code that can use a Reference.unwrap somewhat often, again with the assurance it will be runtime-checked that its loaded, seems fairly pragmatic.

And so now I'm just very minorly complaining that Reference.get() would look nicer than Reference.unwrap(), which I think would be a very trivial/obvious rename except for 1) get(key): Promise<T[K]> is already in the way, and 2) v3 did just get released...so...missed the obvious window on breaking change renames.

[B4nan]

Right, I was also thinking about adding validation there (which would be a BC too). What about a new method, like getEntity(): T, which would check if the entity is initialized and throw if not. And we could also have getProperty(key: keyof T): T[k] that would use getEntity() under the hood.

Then we can rename things in v4, I would like to get there much faster, v3 ended up as much fatter release than I was planning. I am already thinking about requiring TS 3.7 and node 10, and also about splitting the ORM into multiple packages.

[stephenh]

Cool, that all sounds reasonable. Thanks!

[lookfirst]

Node 10 and always latest TS, is fine with me. TS moves so quickly and has been very backwards compatible that I don't see an issue with requiring it.

[lookfirst]

nice...