ember-cli-typescript icon indicating copy to clipboard operation
ember-cli-typescript copied to clipboard

No type-checking on properties passed to createRecord()

Open richard-viney opened this issue 6 years ago • 3 comments
trafficstars

Which package(s) does this enhancement pertain to?

  • [ ] @types/ember
  • [ ] @types/ember__string
  • [ ] @types/ember__polyfills
  • [ ] @types/ember__object
  • [ ] @types/ember__utils
  • [ ] @types/ember__array
  • [ ] @types/ember__engine
  • [ ] @types/ember__debug
  • [ ] @types/ember__runloop
  • [ ] @types/ember__error
  • [ ] @types/ember__controller
  • [ ] @types/ember__component
  • [ ] @types/ember__routing
  • [ ] @types/ember__application
  • [ ] @types/ember__test
  • [ ] @types/ember__service
  • [x] @types/ember-data
  • [ ] @types/rsvp
  • [ ] @types/ember-test-helpers
  • [ ] @types/ember-testing-helpers
  • [ ] Other
  • [ ] I don't know

As a developer using the store's createRecord() API, I want to see compilation errors if I pass an invalid property in as part of the second parameter, e.g.

this.store.createRecord('post', { namme: 'My post' })

Should give an error.

This can be worked around by calling setProperties() separately in order to get the desired error:

const post = this.store.createRecord('post')
post.setProperties({ namme: 'My post' }) // Will see an error here, as expected.

richard-viney avatar Jun 05 '19 03:06 richard-viney

Sorry for the delay, and thanks for reporting this. There's a tricky issue here, in that createRecord will happily accept items which are not part of the schema. If I recall correctly, it won't persist them, but it'll set them on the model object, and that may be desired behavior in some apps.

// my-app/models/person.ts
import Model from 'ember-data/model';
import { attr } from 'ember-data/attr';

export default class Person extends Model {
  @attr('string') name: string;
}
// my-app/controllers/something.ts
import Controller from '@ember/controller';
import { set } from '@ember/object';
import { inject } from '@ember/service';
import DS from 'ember-data';

import Person from 'my-app/models/person';

export default class Something extends Controller {
  @inject store: DS.Store;
  
  person: Person;

  init() {
    super.init();
    set(this, 'person', this.store.createRecord('person', {
      name: 'Chris',
      age: 32
    }));
  }
});
{{! my-app/templates/something.hbs }}
The user is named {{this.person.name}}.<br>
{{#if this.person.age}}
The user is {{this.person.age}} years old.
{{/if}}

Now, I'm sort of back-and-forth in my own mind on this: on the one hand, it's kind of weird to do that, but on the other hand there are valid use cases for it. This is ultimately the result of the conflation of concerns in Ember Data models. As such, my current inclination is to say that the types are annoyingly loose here, but probably correctly so.

I'd be curious get input from the rest of the team here—@dfreeman, @jamescdavis, @mike-north, thoughts?

chriskrycho avatar Jun 25 '19 13:06 chriskrycho

There's a tricky issue here, in that createRecord will happily accept items which are not part of the schema.

That's true of setProperties too, though, isn't it? At runtime it will happily define new properties as well, but we have a restriction on its typing under the assumption that you want to stick to things that are part of the declared type of the receiver.

I think I'd expect createRecord to behave the same way, i.e. in your non-persisted age example, that I would need to define the corresponding model something like this in order for createRecord('person', { name: 'Chris', age: 32 }) to work:

export default class Person extends Model {
  @attr('string') name: string;

  age?: number;
}

dfreeman avatar Jun 25 '19 15:06 dfreeman

Yeah, I was thinking about that later as well. Maybe the right thing to do is default to constraining this the same way we do the others, but providing a blueprint people can use to be looser, as we already do for the registries?

chriskrycho avatar Jun 25 '19 16:06 chriskrycho

This is, as far as I know, unchanged as of 2023, so I am leaving it open. However, it is likely that Ember Data's changes to its APIs will become the solution here when it starts publishing its own types.

chriskrycho avatar Sep 28 '23 22:09 chriskrycho

Ah, as I am doing further triage: this is a duplicate of #251, so closing in favor of that, but the status is the same!

chriskrycho avatar Sep 28 '23 22:09 chriskrycho