ember-m3 icon indicating copy to clipboard operation
ember-m3 copied to clipboard

Deprecate eager transformValue in favor of computeAttribute

Open betocantu93 opened this issue 5 years ago • 4 comments

Hello @hjdivad finally sat down to tackle what we talk about in #810, this is a proof of concept and would love your feedback, there's only a few failing tests but most if not all of them would be solved when we figure out the new api for the m3-schema-manager and the m3-schema

One of my main questions I guess:

Do we proceed to deprecate the schema.models all together? I think its not currently possible because Model depends indirectly on it via schema.isAttributeIncluded for example

https://github.com/hjdivad/ember-m3/blob/1a3390cc9bf3bc4ef2329179bab362dae51eb9fa/addon/model.js#L283

We could keep using the m3-schema-manager as the interface and delegate to the m3-schema service and have some basic examples to implement the same functionality, this would mean more boilerplate... but allows more flexibility

And/Or, we could basically move the m3-schema-manager schema.models details to the m3-schema so we can still support the schema.models and expect users to extend...

This PR also includes the schemaInterface.topModel() method, not sure if that's the best way to reach the topModel from a recordData

betocantu93 avatar Mar 15 '21 21:03 betocantu93

@betocantu93 awesome, thanks for taking this on. I'm very excited to see this evolution -- I think it will make the overall system more flexible and also have a simpler mental model.

hjdivad avatar Mar 17 '21 20:03 hjdivad

Just a checkpoint, for now I removed the hardcoded m3-schema-manager schema.models lookups for optional hooks in the schema, I also created a schema-compat blueprint which implements these hooks to provide an optional "more featured schema" out of the box.

Questions/todos/thoughts I think I need more context

  1. IMHO I'm not entirely sure about removing isAttributeIncluded as it would complicate computeAttribute mental model because now you have to think about when its called or why it was called (by notifyPropertyChange, unknownProperty or setUnknownProprety, the three places that currently use this hook), and also to return schemaInterface.getAttr(key)(?) – I may need more context – but I like the mental model that computeAttribute is used to calculate values only, so I can transform them and then return one of this 4 types ember-m3 support, but that's it. And so, for now... I just left it working as before, hopefully I can get more feedback and implement it in a follow-up PR
  2. I'm not sure how to patch the tests, because there's a few that assert on schema.models semantics, should I deprecate those? or somehow (not sure how) import the schema-compat blueprint and use it?

betocantu93 avatar Apr 24 '21 04:04 betocantu93

Also another after thought about isAttributeIncluded refactor... again I may need more context and this could be part of the implementation details but here

https://github.com/hjdivad/ember-m3/blob/23279ea70e1536074f599346896a01830a06cd33/addon/model.js#L525

Using the proposed computeAttribute returning undefined semantics to stop working or early return on that particular set, notify or get, would mean that the recordData have that data via _attributes, _inFlightAttributes or _data because If I understood correctly, computeAttribute would be expected to return something other than undefined via return schemaInterface.getAttr(key)?, which will not always be the case, specially to allow dynamic sets

betocantu93 avatar Apr 24 '21 05:04 betocantu93

I just refactored a bunch of our MegamorphicModel.reopen stuff for our app with this cleaner API. I also just renamed the blueprints, and so we could close #735 too if this gets merged

betocantu93 avatar Apr 29 '21 23:04 betocantu93