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

Updates to nested change set support. #194

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,10 @@
/coverage/*
/libpeerconnection.log
npm-debug.log*
yarn-error.log
testem.log

# ember-try
.node_modules.ember-try/
bower.json.ember-try
package.json.ember-try
44 changes: 44 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
---
language: node_js
node_js:
# we recommend testing addons with the same minimum supported node version as Ember CLI
# so that your addon works for all apps
- "4"

sudo: false
dist: trusty

addons:
chrome: stable

cache:
yarn: true

env:
# we recommend new addons test the current and previous LTS
# as well as latest stable release (bonus points to beta/canary)
- EMBER_TRY_SCENARIO=ember-1.13
- EMBER_TRY_SCENARIO=ember-lts-2.4
- EMBER_TRY_SCENARIO=ember-lts-2.8
- EMBER_TRY_SCENARIO=ember-lts-2.12
- EMBER_TRY_SCENARIO=ember-release
- EMBER_TRY_SCENARIO=ember-beta
- EMBER_TRY_SCENARIO=ember-canary
- EMBER_TRY_SCENARIO=ember-default

matrix:
fast_finish: true
allow_failures:
- env: EMBER_TRY_SCENARIO=ember-canary

before_install:
- curl -o- -L https://yarnpkg.com/install.sh | bash
- export PATH=$HOME/.yarn/bin:$PATH

install:
- yarn install --no-lockfile --non-interactive

script:
# Usually, it's ok to finish the test scenario without reverting
# to the addon's original dependency state, skipping "cleanup".
- node_modules/.bin/ember try:one $EMBER_TRY_SCENARIO --skip-cleanup
12 changes: 6 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -676,23 +676,23 @@ if (isChangeset(model)) {

## Installation

* `git clone` this repository
* `git clone <repository-url>` this repository
* `cd ember-changeset`
* `npm install`
* `bower install`

## Running

* `ember server`
* Visit your app at http://localhost:4200.
* `ember serve`
* Visit your app at [http://localhost:4200](http://localhost:4200).

## Running Tests

* `npm test` (Runs `ember try:testall` to test your addon against multiple Ember versions)
* `npm test` (Runs `ember try:each` to test your addon against multiple Ember versions)
* `ember test`
* `ember test --server`

## Building

* `ember build`

For more information on using ember-cli, visit [http://ember-cli.com/](http://ember-cli.com/).
For more information on using ember-cli, visit [https://ember-cli.com/](https://ember-cli.com/).
21 changes: 13 additions & 8 deletions addon/-private/relay.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,26 @@ export default EmberObject.extend({
changeset: null,
key: null,

init() {
this._changedKeys = [];
Copy link

@nkgm nkgm Aug 12, 2017

Choose a reason for hiding this comment

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

Keeping track of changed keys on relays doesn't feel right. What if 2 relays modify the same target property? I'm leaning towards the root changeset being the SSOT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a list so that they can be properly notified when a change has occurred. And how can two relays modify the same end property? This is only storing the key of the object.

Copy link

Choose a reason for hiding this comment

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

You're right, I can see now that such condition is not likely to arise. A couple of points on rollback:

  • While I definitely see and have use for it, is it something we want on relays? In the same vein we could add snapshot, validate etc. Thoughts?
  • Couldn't per-relay rollback be implemented on the changeset like this.changeset._rollbackRelay(this.key)? That could also be the way for those other methods.

},

unknownProperty(key) {
return this.getValue(key);
return this.changeset._valueFor(`${get(this, 'key')}.${key}`);
},

setUnknownProperty(key, value) {
this.setValue(key, value);
this._changedKeys.push(key);
this.changeset._validateAndSet(`${get(this, 'key')}.${key}`, value);
this.notifyPropertyChange(key);
return value;
},

getValue(key) {
return this.changeset.valueFor(`${get(this, 'key')}.${key}`);
},

setValue(key, value) {
return this.changeset.validateAndSet(`${get(this, 'key')}.${key}`, value);
rollback() {
for (let key of this._changedKeys) {
this.notifyPropertyChange(key);
}
this._changedKeys = [];
},

destroy() {
Expand Down
Loading