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

Custom Model Classes #487

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@igorT
Copy link
Member

commented May 9, 2019

@tchak

This comment has been minimized.

Copy link
Member

commented May 9, 2019

AttributeSchema -> AttributeDefinition

kind: 'hasMany'| 'belongsTo';
type: string;
options: { [key: string]: any } ;
name: string;

This comment has been minimized.

Copy link
@runspired

runspired May 9, 2019

Contributor

we should validate that this is in fact what we currently expose / want to document as info concerning inversePropertyName inverseType and isPolymorphic is typically available as well.

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

Thats a very good point. isPolymorphic should be available from the options. We currently in practice expose a bunch of underscored properties for inverses, but all of that data is calculable given the exposed public data. I would prefer to keep it private, and figure out in the schema work if we need to expose it


```ts
// Adapter
createRecord(store, type, snapshot)

This comment has been minimized.

Copy link
@HeroicEric

HeroicEric May 9, 2019

I think it would be helpful to provide before & after examples here to show how to implement the changes required

subscribeMetaChanges(identifier, NotificationCallback)
subscribeUnload(identifier, NotificationCallback)
unsubscribe(identifier, NotificationCallback)
}

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 9, 2019

Member

There does not seem to be any discussion about the flow for triggering these callbacks. Presumably the root API is from userland's storeWrapper.notifyX methods but in any case we should call out the flow whatever it is.

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 9, 2019

Member

Also since the classes being given are different, it would be good to have a sense what, if anything is expected to happen between storeWrapper.notifyWhatever and notificationCallback(...)


## Exposing schema information

We currently keep schema information on `DS.Model` class. In order to allow for custom Model implementations we need to allow lookup of schema info from the store. We already have specified a schema api that RecordData consumes: `attributesDefinitionFor` and `relationshipDefinitionFor`. We would define these methods publicly on the store for addons to use. **These methods are not ergonomic on purpose.** They match the current Record Data apis and are designed as a stepping stone on the way of having a better, user facing schema APIs. The schema info is currently primarily geared towards being used by the internal relationship handling layer and the serializer/adapter layers.

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 9, 2019

Member

The schema info is currently primarily geared towards being used by the internal relationship handling layer and the serializer/adapter layers.

What else depends on the schema information?

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

DebugAdapter does.

@igorT igorT changed the title [Data] Custom Model Classes Custom Model Classes May 15, 2019

@igorT

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Action item:

  • Define path forward for snapshots after this rfc
@igorT

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Maybe instead of passing null as a model class, we pass in a shim object for custom model classes to adapter/serializers.

@igorT igorT force-pushed the igorT:igor/custom-model-classes branch from 15c14fd to a638cdf May 22, 2019

Igor Terzic added some commits May 9, 2019

@igorT igorT force-pushed the igorT:igor/custom-model-classes branch from 959be70 to 10ade6f May 22, 2019

Igor Terzic
## Exposing schema information

We currently keep schema information on `DS.Model` class. In order to allow for custom Model implementations we need to allow lookup of schema info from the store. We already have specified a schema api that RecordData consumes: `attributesDefinitionFor` and `relationshipDefinitionFor`. We would define these methods publicly on the store for addons to use. **These methods are not ergonomic on purpose.** They match the current Record Data apis and are designed as a stepping stone on the way of having a better, user facing schema APIs. The schema info is currently primarily geared towards being used by the internal relationship handling layer and the serializer/adapter layers.
We currently keep schema information on `DS.Model` class. In order to allow for custom Model implementations we need to allow lookup of schema info from the store. We already have specified a schema api that RecordData consumes: `attributesDefinitionFor` and `relationshipDefinitionFor`. We would define these methods publicly on the store for addons to use. **These methods are not ergonomic on purpose.** They match the current Record Data apis and are designed as a stepping stone on the way of having a better, user facing schema APIs. The schema info is currently primarily geared towards being used by the internal relationship handling layer, the serializer/adapter layers and the DebugAdapter. Schema methods support both static and dynamic schema computation. For static schemas, the method can always respond with a schema definition based on the type passed, and for dynamic changing schemas, it can look up the underlying data by the identifer which is passed in.

This comment has been minimized.

Copy link
@runspired

runspired May 22, 2019

Contributor

It is slightly concerning to me that we have to introduce schemas in this way here vs through a more complete design (which we want to achieve soon). I do wonder if we shouldn't have targeted designing a schema service first.

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

That's why I designed them in a clunky way so we can replace them later and that most of the usage would be hidden by other public APIs.

The only place you would have to manually deal with them would be in normalization methods.
A medium term solution would be to not deprecate eachAttribute and eachRelationship being passed to normalize, and implement those in the shim class until a fully featured schema service landed

This comment has been minimized.

Copy link
@runspired

runspired May 22, 2019

Contributor

I think shim class implementation is worth exploring


If there was a custom model instance provided, and we had no corresponding class to pass in to `normalizeResponse` we would pass in a shim class that only exposed `modelName` property, thus allowing the normalization layers to continue working with custom model classes.

## How we teach this

This comment has been minimized.

Copy link
@runspired

runspired May 22, 2019

Contributor

I'm a bit concerned about the learning story as we push folks to use Snapshots more and more. Currently they are marked as private API and there is no public way to generate one (for doing things like testing a serializer/adapter). We may want to add a public store.createSnapshot API and work with the learning team on how to properly document classes that are non-user-instantiated but are public API.

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

👍 on documenting, but don't think we need to expose a creation method in order to document. I'm happy to have better snapshot docs as part of this rfc


## Detailed design

After the work in [Record Data Errors RFC](https://github.com/emberjs/rfcs/pull/465), [Record Data State RFC](https://github.com/emberjs/rfcs/pull/463), and [Request State Service RFC](https://github.com/emberjs/rfcs/pull/466) we have enough coverage in public apis that we can implement the entire DS.Model class with just a few changes to Store APIs.

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 22, 2019

Member

Do we have a proof of concept for this in eg something like ember-m3 to make sure we have actually provided enough API to make those use cases possible?

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

Yup, have a spike of both default DS.Model and m3

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 22, 2019

Member

@igorT can we post the spike somewhere public as an example people reviewing it can look at to see what this looks like "in the real world"?

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

Sure thing, I'm in the process of extracting the merged rfcs out of it as well which will make it more readable as well


## Exposing schema information

We currently keep schema information on `DS.Model` class. In order to allow for custom Model implementations we need to allow lookup of schema info from the store. We already have specified a schema api that RecordData consumes: `attributesDefinitionFor` and `relationshipDefinitionFor`. We would define these methods publicly on the store for addons to use. **These methods are not ergonomic on purpose.** They match the current Record Data apis and are designed as a stepping stone on the way of having a better, user facing schema APIs. The schema info is currently primarily geared towards being used by the internal relationship handling layer, the serializer/adapter layers and the DebugAdapter. Schema methods support both static and dynamic schema computation. For static schemas, the method can always respond with a schema definition based on the type passed, and for dynamic changing schemas, it can look up the underlying data by the identifer which is passed in. We would also add a method called `isModelType`, which would return `true` if ED knew that a given string is a model type and `false` otherwise.

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 22, 2019

Member

isModelType, which would return true if ED knew that a given string is a model type and false otherwise.

How would this be determined? Is this intended to be a user extension point?

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

Yup

attributesDefitionFor(identifier: RecordIdentifier | type: string): AttributesDefiniton
// Following the existing RD implementation
relationshipsDefinitionFor(identifier: RecordIdentifier | type: string): RelationshipsDefinition

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 22, 2019

Member

What does it actually look like for a userland model class to provide schema information this way? Are users expected to overwrite these methods in their own store?

The current implementation here has the store knowing something intimate about DS.Model. (in particular that it can read attributes and relationshipsObject from the model class). Should we split the store into the "core" store and "default model userland" store?

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

You would overwrite the methods. If you were doing that, you wouldn't be reading off of the DS.Model presumably. Not quite sure I understand the store split proposal, let's discuss it at the meeting

This comment has been minimized.

Copy link
@runspired

runspired May 22, 2019

Contributor

I would like to split the store.

This comment has been minimized.

Copy link
@runspired

runspired May 22, 2019

Contributor

(That discussion was rapidly approaching and having it now seems a good time)

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 22, 2019

Member

The suggestion is for there to be a store.ts file that does not know anything about DS.Model, in part as a way to dogfood that DS.Model is actually something that can be swapped out.


## Detailed design

After the work in [Record Data Errors RFC](https://github.com/emberjs/rfcs/pull/465), [Record Data State RFC](https://github.com/emberjs/rfcs/pull/463), and [Request State Service RFC](https://github.com/emberjs/rfcs/pull/466) we have enough coverage in public apis that we can implement the entire DS.Model class with just a few changes to Store APIs.

This comment has been minimized.

Copy link
@hjdivad

hjdivad May 22, 2019

Member

I don't see RecordArrayss mentioned. Let's be explicit about the intent here. Can userland model classes participate in RecordArrays? (eg if they're added to record arrays will they automatically be removed when unloaded? If so does their corresponding RecordData have any control over whether or when this happens?)

If we'd like to see RecordArrays tackled separately (and I think that's reasonable) we should call that out in this RFC

Igor Terzic
@igorT

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

Action item: Talk about Record Arrays

}
```

For backwards compatibility, we would still lookup the class from the registry, and if we found a class we would return it with a deprecation, but would no longer error if null was returned. If we did not find the class, we would create a shim class that exposed a deprecated `modelName`. We would deprecate accessing the class when passed to serializer/adapter by wrapping it in a proxy in dev mode. Currently Snapshots contain all of the data that is available on the Model class which would be needed for serializing/normalizing.

This comment has been minimized.

Copy link
@igorT

igorT May 22, 2019

Author Member

Misleading, fix deprecation prose

class Store {
// Following the existing RD implementation
attributesDefitionFor(identifier: RecordIdentifier | type: string): AttributesDefiniton

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 22, 2019

Member
Suggested change
attributesDefitionFor(identifier: RecordIdentifier | type: string): AttributesDefiniton
attributesDefinitionFor(identifier: RecordIdentifier | type: string): AttributesDefiniton
@igorT

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

Add the overall store signature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.