adopted-ember-addons / ember-data-model-fragments

Ember Data addon to support nested JSON documents
MIT License
370 stars 114 forks source link

chore: fix typescript declarations #445

Closed charlesfries closed 2 years ago

charlesfries commented 2 years ago

This PR fixes some type string vs type class inconsistencies in the TypeScript declaration files from #443

444

knownasilya commented 2 years ago

@charlesfries this ready for review?

charlesfries commented 2 years ago

@knownasilya it is ready for review now. I was taking another look at createFragment and I realized why the original PR author possibly went with string keys rather than real fragment classes in the fragment generics--because afaik we can't start with the fragment class and get back to the string key, then over and up to the fragment attributes interface from its registry. if we start with just the key we can easily work our way to both registries

however I personally am not really in love with the idea of having to define a separate interface just for defining the raw structure of a fragment. I have seen Partial<T> used in other libs for this kind of problem, and so I have updated the PR to remove the attribute registry functionality in favor of Partial. lmk your thoughts

VincentMolinie commented 2 years ago

@knownasilya it is ready for review now. I was taking another look at createFragment and I realized why the original PR author possibly went with string keys rather than real fragment classes in the fragment generics--because afaik we can't start with the fragment class and get back to the string key, then over and up to the fragment attributes interface from its registry. if we start with just the key we can easily work our way to both registries

however I personally am not really in love with the idea of having to define a separate interface just for defining the raw structure of a fragment. I have seen Partial<T> used in other libs for this kind of problem, and so I have updated the PR to remove the attribute registry functionality in favor of Partial. lmk your thoughts

The thing is you don't have the real type for the createFragment as you might pass any properties from the fragment class (not any attributes like it should be)