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

subscribe is never called with the initial value on created but not set records? #138

Closed
ronag opened this issue May 31, 2016 · 10 comments
Closed
Assignees
Labels
Milestone

Comments

@ronag
Copy link

ronag commented May 31, 2016

e.g.

const rec = ds.record.getRecord(id)
rec.subscribe(function (value) {
  console.log(value)
}, true)

Given that records are create on getRecord I would expect the above code to print {}...

Or is the record state after getRecord something in between exists and doesn't exists?

@ronag ronag changed the title subscribe is never called with the initial value? subscribe is never called with the initial value on created but not set records? May 31, 2016
@yasserf
Copy link
Contributor

yasserf commented May 31, 2016

getRecord is async if the record isn't already loaded client side, which means you won't have the data from the server immediately afterwards when you do a subscribe( fn, true )

@ronag
Copy link
Author

ronag commented May 31, 2016

@yasserf: Yes, but shouldn't it eventually print {}? Because if I do a snapshot I will get {}... so it kind of exists but doesn't...? There seems to be an inconsistency between subscribe and snapshot in this scenario.

@yasserf
Copy link
Contributor

yasserf commented May 31, 2016

This is got to do with it just being async

const rec = ds.record.getRecord(id) <- async call to get data
rec.subscribe(function (value) {
  console.log(value)
}, true) <- immediate callback that gets triggered before record is created
const rec = ds.record.getRecord(id) <- async call to get data
ds.record.snapshot( function() { 
} ) <- also async, hences gets called with data

@yasserf
Copy link
Contributor

yasserf commented May 31, 2016

The subscribe method should print {} once the initial data comes back though. Is that not the case?

@ronag
Copy link
Author

ronag commented May 31, 2016

Is that not the case?

No it's not...

@ronag
Copy link
Author

ronag commented May 31, 2016

const rec = ds.record.getRecord(id) <- async call to get data
ds.record.snapshot( function() { 
} ) <- also async, hences gets called with data

It's not async, see: https://github.com/deepstreamIO/deepstream.io-client-js/blob/master/src/record/record-handler.js#L138

It should be, at the least:

RecordHandler.prototype.snapshot = function( name, callback ) {
    if( this._records[ name ] ) {
        setImmediate(callback.bind(null, null, this._records[ name ].get() );
    } else {
        this._snapshotRegistry.request( name, callback );
    }
};

Though, that is a different issue/bug.

I would also argue that this would be less confusing:

RecordHandler.prototype.snapshot = function( name, callback ) {
    if( this._records[ name ] && this._records[ name ].isReady ) {
        setImmediate(callback.bind(null, null, this._records[ name ].get() );
    } else {
        this._snapshotRegistry.request( name, callback );
    }
};

Since it would decouple the behaviour from getRecord.

@ronag
Copy link
Author

ronag commented May 31, 2016

The reason this is an issue for us is that we now have to use a rather silly implementation:

observe (recordName, path) {
    const record = ds.record.getRecord(recordName)
    return Observable
      .create(o => {
        record.whenReady(() => {
          // Since subscribe with triggerNow will not give the initial value for created but not set records...
          o.next(record.get(path))
          o.complete()
        })
      })
      .concat(Observable
        .create(o => {
          const update = (value) => o.next(value)
          record.subscribe(path, update)
          return () => {
            record.unsubscribe(path, update)
            record.discard()
          }
        })
      )
  }

@yasserf
Copy link
Contributor

yasserf commented May 31, 2016

Yup your totally right, snapshot should only be sync if ready. If you raise a bug well'll make sure to fix it for the next release.

@ronag
Copy link
Author

ronag commented May 31, 2016

Of course I'm right. I've been reading this book:

51i2p03gxhl sx324_bo1 204 203 200

I'll make a separate issue.

@ronag
Copy link
Author

ronag commented May 31, 2016

@yasserf: The problem is here: https://github.com/deepstreamIO/deepstream.io-client-js/blob/master/src/record/record.js#L514

If the record is created then the oldValue will be {} thus subscribe will not receive it... and we have a problem...

@yasserf yasserf added the bug label Jun 17, 2016
@yasserf yasserf added this to the 1.0 milestone Jun 17, 2016
@yasserf yasserf added the ready label Jun 17, 2016
@timaschew timaschew added roadmap and removed ready labels Jul 20, 2016
yasserf added a commit that referenced this issue Jul 27, 2016
@yasserf yasserf self-assigned this Jul 27, 2016
yasserf added a commit that referenced this issue Jul 27, 2016
yasserf added a commit that referenced this issue Jul 27, 2016
yasserf added a commit that referenced this issue Jul 27, 2016
yasserf added a commit that referenced this issue Jul 27, 2016
yasserf added a commit that referenced this issue Jul 27, 2016
yasserf added a commit that referenced this issue Jul 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants