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

Improve model functionality #88

Merged
merged 3 commits into from
Feb 12, 2017

Conversation

spzm
Copy link
Contributor

@spzm spzm commented Feb 3, 2017

There are few improvements are proposed in the PR:

  1. Always create an object for attributes field when creating a Model. It will prevent situation when the single value or undefined can be passed to it.

  2. Introduce the get method as a common approach to retrieve a value from attributes field. The get method support complex keys and actually uses lodash get.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3be7dac on spzm:improve-model-implementation into 382918f on Travix-International:master.

src/Model.js Outdated
@@ -2,7 +2,14 @@ import _ from 'lodash';

export default class Model {
constructor(attributes) {
this.attributes = attributes;
this.attributes = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... why not make it a one line:

this.attributes = Object.assign({}, attributes);

?

However, bear in mind that if you have nested objects, you will still maintain the reference to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I have update. Regarding references, the alternative solution is cloneDeep but it can cause the performance. I think it is matter of discuss.

get(key) {
if (typeof key !== 'string') return undefined;

return _.get(this.attributes, key);
Copy link
Contributor

@mAiNiNfEcTiOn mAiNiNfEcTiOn Feb 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// on top of the file
const FIELD_SEPARATOR = '.';

// in the model
  get(key) {
    if ((typeof key === 'string') && (key.length)) {
      return key.split(FIELD_SEPARATOR).reduce((ret, field) => {
        if ((typeof ret === 'object') && (ret !== null)) {
          return ret.hasOwnProperty(field) ? ret[field] : undefined;
        }
        return ret;
      }, this.attributes);
    }
    return undefined;
  }

(Ignore this message, I just hate lodash usage for such small behaviour 😝 )

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 978d5da on spzm:improve-model-implementation into 382918f on Travix-International:master.

@fahad19
Copy link
Member

fahad19 commented Feb 6, 2017

Can you please update the docs too?

Mainly:

  • Update docs/Api/createModel.md
  • May be create a new docs/Api/Model.md file too since this is an instance method

@alexmiranda
Copy link
Contributor

Is this really necessary? I mean, why would we be concerned with creating nesting and structures into models whilst most of its use it to access static readonly data provided by parent/root application? I understand that for representing some attributes it may be reasonable that you group some attributes e.g. address, pricing, etc; however, this is almost never the case with the fireball applications that we have, i.e. models support configuration and settings injected into components. If we introduce models to carry data from booking flow (more structured, dynamic data), for instance, why would you want to complicate access pattern if you can very well use a well known json object/schema?

This is not to say I'm against this in any way. If you have any specific use case that would really benefit from this, bring it up. I'd be glad to hear. Other than that, YAGNI.

@mAiNiNfEcTiOn
Copy link
Contributor

mAiNiNfEcTiOn commented Feb 6, 2017

@alexmiranda Although the 'nested get' is not priority, we should definitely have into consideration the fact that the attributes are being defined by reference:

// MyModel.js
import { createModel } from 'frint';
export default createModel({
  set(key, value) {
    this.attributes[key] = value;
  }
});

// Dependant
import MyModel from './MyModel';

const obj = {
  prop: 'value',
  nested: {
    prop: 'nested value',
  },
};

const model = new MyModel(obj);
model.set('prop', 'new value');

console.log(obj.prop === 'new value'); // true

Although this is an harmless case, you can get the point when you're using a parent model (e.g. Configuration), and the apps decide to set stuff :)

@spzm
Copy link
Contributor Author

spzm commented Feb 6, 2017

My main goal was to resolve the issue when you make new MyModel(undefined), then empty attributes objects will be created. Then in initialize method of createModel I have to check - is it object or not. Also the case when the argument is any other primitive type.

The getter idea become useful when we get the values from the model. Also it help to understand that all the model data have to be stored in attributes. The getter also can be useful a lot inside model methods.

Also one of my idea was to freeze model object and attributes after creation. Then it will more read only. In my view, model was something that I create ones when data comes. And then using my model as read only storage. It how we used it in +/- 3 days widget. I'm curious about real examples when we need to change the model after creation.

Model changes can make the structure of model data quiet different. Then it can make a lot of issues when I using it. It would be good if the model will be something that I can trust (structure and types I mean).

@mAiNiNfEcTiOn mAiNiNfEcTiOn merged commit 3d6c7ce into frintjs:master Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants