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

Internal Models and model expansions. #432

Merged
merged 4 commits into from
Jun 14, 2017

Conversation

agonza40
Copy link
Contributor

This project uses semver, please check the scope of this pr:

  • #none# - documentation fixes and/or test additions
  • #patch# - backwards-compatible bug fix
  • #minor# - adding functionality in a backwards-compatible manner
  • #major# - incompatible API change

CHANGELOG

Changed model expansions and internal models to take effect in nested objects.
Added support for internal models within arrays.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9ce6054 on agonza40:internal-models into ** on ciena-frost:master**.

const view = newProps.view ? this.getRenderView(model, newProps.view) : this.get('renderView')
const view = newProps.view
? this.getRenderView(model, newProps.view)
: this.getRenderView(model, this.get('view'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the view normalization is performed all the time on every store update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was having trouble getting the new normalized view from the computed property in the cases where I needed it. I think this can all be smoothed out with some slight restructuring of this component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, lets hope this doesn't cause a performance regression. Can you double-check that the EVC example still runs smoothly with this change?

addon/utils.js Outdated
if (key === '_internal') {
return key
}
if (['boolean', 'number', 'string', 'undefined', 'null'].indexOf(typeof item) !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been using [].includes to check existence in an array more than indexOf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I was looking for! I was trying to use [].contains() which isn't a real thing.

addon/utils.js Outdated
if (subPaths.length <= 0) {
return
}
return _.map(subPaths, function (subPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason you're favoring _.map over map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may have originally thought subPaths could be something other than an array.

addon/utils.js Outdated
return `${key}.${subPath}`
})
})
.filter()
Copy link
Contributor

Choose a reason for hiding this comment

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

So what does a filter with no args do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filters out falsey values.

@sophypal
Copy link
Contributor

sophypal commented Jun 14, 2017

👍

Approved with PullApprove

@sophypal
Copy link
Contributor

sophypal commented Jun 14, 2017

👍

Approved with PullApprove

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 47535f4 on agonza40:internal-models into ** on ciena-frost:master**.

@agonza40 agonza40 merged commit f3f0297 into ciena-frost:master Jun 14, 2017
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

Successfully merging this pull request may close these issues.

4 participants