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

allow array to be updated using obs.set #12

Merged
merged 1 commit into from
Oct 5, 2014
Merged

allow array to be updated using obs.set #12

merged 1 commit into from
Oct 5, 2014

Conversation

mmckegg
Copy link
Collaborator

@mmckegg mmckegg commented Aug 21, 2014

Currently obs.set simply falls back to observ's notify method, so if you call set on an observ-array everything just breaks. Trouble is, it appears to work, but the inner ._list is never updated. This behavior is not really ideal and quite confusing.

The new transaction method just about does what we want here, minus the initial shallow copy. This PR extracts out the update portion of transaction to a new obs.set method allowing the inner ._list to be updated using obs.set(rawList).

@Raynos I think this behavior of set makes more sense. What do you think?

extract update portion of `transaction` to standalone set method
@afandria
Copy link

I agree. Defining set directly on observ-array would make the API be more consistent.

I fell into the trap of using obsArray.set(rawArray). Some behaviors worked, but some methods such as getLength() were returning the wrong values.

@Raynos
Copy link
Owner

Raynos commented Aug 27, 2014

This PR implements a subset of #9

We want to support this.

@mmckegg
Copy link
Collaborator Author

mmckegg commented Oct 5, 2014

Could really use this right now.

This PR doesn't support merging in nested observables (instead indices are overridden by the exact data set). This is tricky to support with arrays unless there's a way to uniquely identify the each element. The index isn't good enough as this moves around with splice operations.

This also means two-way databinding (at least with nesting) is not possible with this approach.

I seem to recall @Raynos discussing value only ObservArray (or was it Struct?). Maybe this obs.set would make more sense there.

@Raynos Shall we merge or should I fork?

@mmckegg
Copy link
Collaborator Author

mmckegg commented Oct 5, 2014

Or maybe set would be better as a reinitializer? Essentially the same as creating a new ObservArray, throwing away the internal _list, and using the supplied one. Wouldn't get any diffing this way, but at least it would make the various obs.functions work as expected after an obs.set.

function set(array) {
    var obs = this
    obs.removeListeners.forEach(invoke)
    obs.removeListeners = list.map(function (observ) {
        return typeof observ === "function" ?
            addListener(obs, observ) :
            null
    })
    obs._list = array

    var values = array.map(function(value){
        return typeof value === "function" ? value() : value
    })

    obs._observSet(values)
}

@Raynos
Copy link
Owner

Raynos commented Oct 5, 2014

lgtm, feel free to merge.

We need a better way of doing .set()

mmckegg added a commit that referenced this pull request Oct 5, 2014
allow array to be updated using `obs.set`
@mmckegg mmckegg merged commit 02ced0f into master Oct 5, 2014
@mmckegg
Copy link
Collaborator Author

mmckegg commented Oct 5, 2014

@Raynos I don't seem to have npm publish rights. Can you publish this for me? I guess we should probably bump major.

@Raynos
Copy link
Owner

Raynos commented Oct 5, 2014

Published v3.0.0 :)

@mmckegg mmckegg deleted the better-set branch October 7, 2014 13:52
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.

None yet

3 participants