dualsync remote/local options confusing

[scooter12]

For models where both properties local and remote are specified, the case where local==true and remote==true causes function dualSync to only execute the onlineSync method.

  dualsync = function(method, model, options) {
    var response, success;
    console.log('dualsync', method, model, options);
    options.storeName = result(model.collection, 'url') || result(model, 'url');
    if (result(model, 'remote') || result(model.collection, 'remote')) {
      return onlineSync(method, model, options); //////<----------Case where local==true is skipped since we're returning
    }
    if ((options.remote === false) || result(model, 'local') || result(model.collection, 'local')) {
      return localsync(method, model, options);
    }

My suggested replacement, which also allows you to pass options.local as a working parameter and make it a little bit more readable is:

  dualsync = function(method, model, options) {
    var response, success;
    var remote, local;
    console.log('dualsync', method, model, options);
    options.storeName = result(model.collection, 'url') || result(model, 'url');

    remote = result(model, 'remote') || result(model.collection, 'remote') || (options.remote === true);
    local =  result(model, 'local') || result(model.collection, 'local') || (options.local === true);

    if ( remote && !local ) {
      return onlineSync(method, model, options);
    }
    if ( local && !remote ) {
      return localsync(method, model, options);
    }

[thiagobc]

local and remote options are meant to use only when you want local OR remote.

To use both (dualStorage default behavior), simply remove local: true and remote: true configurations.

[apaatsio]

My suggestion is to combine local and remote options into single storageMode option. This could have values 'local', 'remote', or 'default'. I think it would clarify the options.

[lucian1900]

@apaatsio Yes, something like that might indeed be clearer.

[reubano]

+1
having both options is confusing and unnecessary. I also noticed fetch {remote: false} works (fetches locally not from server) while fetch {local: true} doesn't.

[ryno1234]

+1

[nilbus]

I would accept a pull request that makes these options work in a more sensible way.

[elad]

+1.

I believe this whole aspect requires auditing and possibly polishing up. I'd like to be able to force remote/local on individual requests, and I'd like to be able to tell dualStorage to discard dirty models.

I'm now dealing with a behavior that in my opinion is a major bug: when I start my app and issue a fetch, because there are stray dirty models in a collection it always fetches from localStorage, even though the app is online. There is no indication this is happening and the actual data is corrupt due to the id generated internally.

I'll try to find some time to fix these things myself.

(If this is too off for this issue, please let me know and I'll open a new one.)

[nilbus]

Yes, I think this is deserving of discussion in a new issue. I appreciate
your contributions.

[nilbus]

@eladxxx in elad@4f7bd41 (currently unmerged) added the ability to specify remote: true and local: true in the options for any sync operation. I'm trying to reconcile this with the current behavior, which would be:

• local only and mark dirty
  • options.remote = false
• local only but don't mark dirty
  • model/collection local attribute/method is truthy
  • (new) options.local = true
• remote only, and don't mark dirty
  • model/collection remote attribute/method is truthy
  • (new) options.remote = true

My thought is that the new options.local option probably should cause things to be marked dirty, but I'm unsure what the use case of "local only but don't mark dirty" is. @thiagobc and @lucian1900, do you know the use case where you would want to save locally but not mark it dirty? It has been this way since @thiagobc started marking dirty records.

[lucian1900]

The only potential use case might be data you never wish to sync, but
perhaps a different local persistence mechanism may work better.

It may make sense to collapse the two options into a single enum-like flag,
as some have suggested.

On Saturday, 10 May 2014, Edward Anderson [email protected] wrote:

@eladxxx https://github.com/eladxxx in elad/Backbone.dualStorage@4f7bd41elad@4f7bd41bb2792a128810403e44acc50a83e07304(currently unmerged) added the ability to specify remote:
true and local: true in the options for any sync operation. I'm trying to
reconcile this with the current behavior, which would be:

• local only and mark dirty
  • options.remote = false
    • local only but don't mark dirty
  • model/collection local attribute/method is truthy
  • (new) options.local = true
    • remote only, and don't mark dirty
  • model/collection remote attribute/method is truthy
  • (new) options.remote = true

My thought is that the new options.local option probably should cause
things to be marked dirty, but I'm unsure what the use case of "local only
but don't mark dirty" is. @thiagobc https://github.com/thiagobc and
@lucian1900 https://github.com/lucian1900, do you know the use case
where you would want to save locally but not mark it dirty? It has been
this way since @thiagobc https://github.com/thiagobc started marking
dirty records.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/12#issuecomment-42732107
.

[thiagobc]

The use case was exactly as @lucian1900 explained, local-only data.

So, the logic would be: options.local should mark things as dirty if model.remote option is true.

[nilbus]

If data is local only, I can't see any harm in marking it dirty, as that would just cause it to always work locally. Then if the model later changes to not be local-only, the local data will already be marked dirty and require a sync.

As it is now, dualstorage would incorrectly assume that the local data is in sync with the remote. We don't explain in the readme that setting local: true assumes that you will never sync online. In fact, I'm sure many including myself have made the opposite assumption and use that as a temporary flag.

Any objections to moving to this?

• local only, and mark dirty
  • options.local = true
  • options.remote = false
  • model/collection local attribute/method is truthy
• remote only, and don't mark dirty
  • options.remote = true
  • options.local = false
  • model/collection remote attribute/method is truthy
• dualsync
  • options.local = true and options.remote = true
  • nothing specified
• error
  • options.local = false and options.remote = false
• options override model/collection attribute/method

Is this too complex? Is it more intuitive?

It would be nice to make it a single option/attribute/method, but that will require a major API bump.

[elad]

Hello,

I don't mean to interrupt, but I would just like to add that I find the above a bit too complex. I don't think there's a way I could remember all of that and the fact we need to clarify what each combination does might be a hint that there are too many combinations. :)

Personally, I'm not sure there's a use case for all combinations. For example in my app the only flag I really wanted was remote on a sync operation. The use case is for an "import" feature where unlike other areas, if the data cannot be persisted to the server, the operation must be declared failed.

If this aspect is to be polished, at the very least the remote/local flags should be a single parameter whose function is much easier to understand, and maybe even the number of combinations should be reduced based on real-world use cases.

[nilbus]

I agree that it seems complex. My foremost goal for the near-term is to not break the existing API, and also make it not confusing.

Maybe the right thing to do instead is deprecate (and keep until the next major version) the local and remote options and attributes, and add a single option/attribute that replaces the two, called storageMode (as suggested by @apaatsio) with modes 'offline', 'remote-only', and 'default'. Alternatively we could use 'local', 'remote', and 'default' (or 'dual'). I think 'offline' instead of 'local' helps imply that we will mark records dirty, and that it will act the same way as it would when offline. I don't believe we need any mode that uses local storage does not mark records dirty.

Please give me your thoughts on my proposal and these new names.

[reubano]

Agreed. If you want to change the dirty model behavior then there should be an explicit option to deal with it. E.g., something like markDirty. @nilbus if you are worried about maintaining backwards compatibility, you can always do something like this:

if options.storageMode is 'local' or options?.local
  storageMode = 'local' 
else if options.storageMode is 'remote' or options?.remote
  storageMode = 'remote'
else
  storageMode = 'default'

[elad]

I agree, and unless someone comes up with a use case for non-dirty local records, I don't think such a mode should be supported. As for naming, I like offline for the same reasons you do and prefer remote because it's short.

I also think @reubano's backward compatibility layer is a good idea.