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

path forward for Class.modelName #4814

Closed
3 of 9 tasks
stefanpenner opened this issue Feb 22, 2017 · 1 comment
Closed
3 of 9 tasks

path forward for Class.modelName #4814

stefanpenner opened this issue Feb 22, 2017 · 1 comment

Comments

@stefanpenner
Copy link
Member

stefanpenner commented Feb 22, 2017

Why?

Polluting classes with a name has some issues, these issues include: sharing of models via reexporting, or manual registration, or custom resolver does not work correctly. In general is bad form to mutating "global" state.

Specifically, we would like move away from having to re-extend classes constantly defensively to prevent the above described issues.

usages:

  • isPrimaryType: remove isPrimaryType’s reliance on modelName #4832

  • _findInverseFor: meta.parentType.modelName [PERF] Model.prototype.didDefineProperty only in dev #4924
    meta.parentType appears to exist purely for a runtime warning. We may be able to remove it, or turn it into a development only thing?

    • -private/system/relationships/ext.js:54 (assertion)
    • -private/system/relationship-meta.js:21 (just incidental)
    • -private/system/model/model.js:1084 (for a warning)
  • findPossibleInverses type.modelName (superclass stuff)
    Jumping between name and constructor to have inheritance aware automatic inverses. They relationship map is keyed on modelName, but the inheritance is obviously classes (needs more experimentation)

  • createRecord

  • serializeIntoHash
    type leaks into the public API, but type and modelName aren't 1:1. Maybe we also provide modelName? or..

    • addon/serializers/rest.js
  • _getMappedKey via serializeAttribute / serializeBelongsTo / normalize

  • updateRecord addon/adapters/rest.js
    this.buildURL(type.modelName
    we pass type, in this case only for modelName. We should consider modelName only, as a user can lookup model if they need.

  • query
    same as ^

  • adapter public AP I( like findRecord etc)

  • ...

thoughts:

  • in some cases, its easily avoidable (lets do that)
  • in others, it is based on our usage of passing around our classes themselves. There are solutions to this, one of which would be to create a very similar object that is more like what @runspired describes as a schema.
  • is there a transition path to move away from type to something else? Like the string.... or another thing...

More to come !

@runspired
Copy link
Contributor

now that we have schemas (and also now that we handle the factory and extend differently) this is not an issue. The final death of this will be removing support for @ember-data/model more generally which is planned for 5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants