Optionally preserve existing url on sending routed signal with upload.

[freele]

This would allow to implement modules each one of which will add its part to url, not disturbing the rest part of the url.
For example, I have 'pagination' module and 'search' module. They both have to add some parameters to the query in the url. But now for preserving the url I have to keep it in my state and merge it with new upload before sending routed signal (and merged version goes as upload).
Maybe it can be useful to add a setting to the router, which will keep url and merge signal's upload to existing url rather than overwriting it?

[christianalfoni]

Hm, I do not think I completely understood the scenario. Do you have some small code snippets? Where you define the url, trigger the signal and where you store the state? :-) @Guria should probably also get in on this

[freele]

Triggering events in the controller with pagination:

  onLimitChange = (e, index, limit) => {
    this.props.signals.onLimitChanged({ limit });
    // queryJSON - object from state, it's parsed with URLON current url.
    // here I merge it with new query params, to preserve other parts of query (e.g. search string)
    this.props.signals.pageUsersOpened(Object.assign({}, this.props.queryJSON, { page: 1 }, { limit }));
  }
  onPageChange = (page) => {
    if (page > 0 && page <= this.props.pagination.pagesAmount) { // in case of glitches with button deactivation
      this.props.signals.onPageChanged({ page });  
      // and here I want to save 'limit' part of query. without merging there will be only number of page
      this.props.signals.pageUsersOpened(Object.assign({}, this.props.queryJSON, { page }));
    }
  }

Thank you for the response.
If it's still unclear I will cleanup my code and add some clear example of what I mean.

[Guria]

I'll try to look forward with this in nearest time.

[christianalfoni]

Aha, so if I understand this correctly. You have a url with query ?search=whatever and when you trigger a signal: signals.something({limit: 20}), you want the url to be ?search=whatever&limit=20, not ?limit=20 as it is now?

[freele]

Yes, exactly.

[Guria]

I had thoughts on this case. Would present my suggestion soon.

[freele]

It would be great, for me it is pretty typical use case.
Maybe a good way to do it is to set up some prefix for preservable parameters of the query. If, for example, we take double underscore (__) as a prefix, then ?__search=whatever&page=2 will mean that page will change after url update (signals.something({limit: 20})), but search will be the same.

[Guria]

I read more careful. Seems I was thought about slightly different case, like global query parameter (?lang=ru). It could be solved with mapping such params to some state path directly.

Router(controller, routes, {
  globalParams: {
    lang: ['lang']
  }
})

Then router would exclude globalParams from passing as signal payload and would add it while stringify. Not sure if it would fit your case.

Can you tell more about splitting pagination and search to separate modules?
Cause I see that both search-term and page number parameters could fit naturally to one signal.

[freele]

Mapping query parameters to state path is a good idea, I think. But it's important to keep correct and simple flow of data of cerebral. I mean, to update state in a right moment. Maybe it will be simpler to manage dehydration and rehydration of the state with such mapping (it's other story I suppose...).

Regarding search or filters - this parameters may depends on data that is shown in a table in a page. Pagination defines just the portion of rows that is shown right now. Actually, it's just my case, maybe it also may depend on API structure, for example. Either way, I think it's good to let some modules to control their part of url.

[Guria]

Why you have separate onLimitChanged and onPageChanged signals? I would suggest just add action chains for setPage, setLimit, setFilter into your pageUsersOpened signal. So pageUsersOpened signal definition would have complete picture of what will happen.
Also router bound only to one signal. It wouldn't be able to call both onLimitChanged and pageUsersOpenedlike you do in your callback.
I could make more concrete suggestion if you could share a little more details. Feel free to contact me privately.

I think it's good to let some modules to control their part of url

As I said above, it is hard to acheive it because route is bound to one signal a time.

[Guria]

Moved out global params as separate issue to discuss #58

[christianalfoni]

@Guria and @freele , yes, if you have a query in your url that has information about the state I would suggest creating a signal that supports it. So:

controller.signal('tableQueryChanged', chain);

And:

onPageChange(page) {
  const limit = this.props.limit;
  const filter = this.props.filter;
  this.props.signals.tableQueryChanged({limit, page, filter})
}
onLimitChange(page) {
  const page = this.props.page;
  const filter = this.props.filter;
  this.props.signals.tableQueryChanged({limit, page, filter})
}

And your chain sets all of them every time. The way the router works conflict with how we traditionally think sometimes. It is interesting to look at these scenarios and how we can solve them :-) Lets just iterate on this and I am sure we will find a common ground!

[freele]

@Guria, thank you. Yes, in my case state is updated in pageUsersOpened signal. And there will be inconsistency if I will load the page from url, and it should be fixed.
@christianalfoni, yeah you're right, and, as I just wrote, it should be done this way.
But there also may be modules which modifies state of the view in different signals, but in common way. Like pagination. Pagination module can be the same for users list and for songs list. And both signals onUserListChanged and onSongListChanged can read this query params from the query (like limit and page). What I mean is, if query is not used for matching route path, it can be used by different modules.
Of course, I'm not sure that it is really crucial, but for now I don't see much contradiction of this idea with the existing router.

[Guria]

Main idea that signals shouldn't relay on payload source. It just receive it no matter if it from url or direct call. But app developer is free to define how signals payload mapped to url.
Should we allow to compose url more then from just one signal still good question to discuss.

[christianalfoni]

Hm, yeah. I am just very unsure how to keep this simple. I think maybe we should expose a new method on signals bound to the router. Like:

// Instead of, which overrides
this,props.signals.pageChanged({page: 12});
// You would, which keeps current query
this,props.signals.pageChanged.keepQuery({page: 12});
// Or expose an option object
this,props.signals.pageChanged({page: 12}, {
  keepQuery: true
});

Really bad name, but that way it is explicit. Instead of passing options somewhere else. It is very likely that you debug the signal fired to understand how the url was created, so it is a good place to put it.

If it covers all scenarios

[christianalfoni]

As this is being discussed just adding:

this.props.signals.someSignal({page: 12}, {mergeQuery: true});

// Or
module.addSignals({
  someSignal: {
    chain: [],
    mergeQuery: true
  }
})

I think this is a good convention of adding any options to signals. You can add it "on the fly" or you can set it on the signal definition. This allows modules affected to hook on to the options as well to react to them. Meaning that modules would be able to empower signals with new options.

Like in this example, the router does

[Guria]

@christianalfoni I suppose that we will need to know which query should be merged and which not. Eg, while navigating from one module to another is most likely we will need to replace query.

[maxfrigge]

Like the idea of having a unified way for modules in general!

Maybe those options should be wrapped in parameter?

module.addSignals({
  someSignal: {
    chain: [],
    options: {
      mergeQuery: true
    }
  }
})

[Guria]

@maxfrigge @christianalfoni says that chain is also option. And it already works like this.

[maxfrigge]

Oh.. i thought its the object way of setting a signal and chain is the action chain. Isn't this how you define sync signals with new api?

[Guria]

exactly. it is the same syntax:

module.addSignals({
  someSignal: {
    chain: [action1, action2],
    mergeQuery: true,
    isSync: true
  }
})

[maxfrigge]

Ah.. I understood that by setting:

module.addSignals({
  someSignal: {
    chain: [],
    options: {
      mergeQuery: true
    }
  }
})

You define that when calling the signal in a view.. you can pass the option mergeQuery which defaults to true. And use that as a generic way that allows all modules have options passed along with signals.

The way you are proposing it it's "just a router thing", but wouldn't it be good to have a generic way to deal with such things so that it's not tied to the router module. Or would this still be possible?

[Guria]

The only difference is you are adding extra options property. @christianalfoni said me that every property in object used as signal definition is already option. Even chain property with actions is also option. ^)

[maxfrigge]

ok. cool. than stick with that :)

[christianalfoni]

Just confirming that object is "options". So can attach whatever we want directly to root of object :-)

[christianalfoni]

What is the status here now? :)

[Guria]

Planned to make expiremetal implementation.

[Guria]

function wrapSignal(signal) {
  var prevPayload = {}
  return function (payload, options) {
    if (options.merge) {
      payload = Object.assign({}, prevPayload, payload)
    }
    prevPayload = payload
    delete options.merge
    signal(payload, options)
  }
}

someGridOpened = wrapSignal(this.props.signals.someGridOpened)

someGridOpened({
  page: 0,
  filter: '',
  sort: [
    { field: 'fieldA', dir: 'ASC' }
  ]
})

someGridOpened({ filter: 'bla bla' }, { merge: true })

Pros

• Query would contain merged values
• Nothing to change in router
• Work without router
• Could be extracted to helper module

Cons

• Doesn't work for links

[Guria]

function wrapSignal(signal) {
  var prevPayload = {}
  function wrappedSignal (payload, options) {
    if (options.merge) {
      payload = Object.assign({}, prevPayload, payload)
    }
    prevPayload = payload
    delete options.merge
    signal(payload, options)
  }

  wrappedSignal.merge = function (payload) {
    return Object.assign({}, prevPayload, payload)
  }

  return wrappedSignal
}

someGridOpened = wrapSignal(this.props.signals.someGridOpened)

someGridOpened({
  page: 0,
  filter: '',
  sort: [
    { field: 'fieldA', dir: 'ASC' }
  ]
})

someGridOpened({ filter: 'bla bla' }, { merge: true })

<Link signal={this.props.signals.someGridOpened} params={ someGridOpened.merge({ page: 1}) }>
  Next Page
</Link>

Pros

• same of previous
• added links support

[Guria]

function wrapSignal(signal) {
  var prevPayload = {}
  function wrappedSignal (payload, options) {
    if (options.merge) {
      payload = Object.assign({}, prevPayload, payload)
    }
    prevPayload = payload
    delete options.merge
    signal(payload, options)
  }

  wrappedSignal.signalName = signal.signalName

  return wrappedSignal
}

someGridOpened = wrapSignal(this.props.signals.someGridOpened)

someGridOpened({
  page: 0,
  filter: '',
  sort: [
    { field: 'fieldA', dir: 'ASC' }
  ]
})

someGridOpened({ filter: 'bla bla' }, { merge: true })

<Link signal={someGridOpened} params={{ page: 1 }} options={{ merge: true }}>
  Next Page
</Link>

Pros

• same of previous
• link support seems just a little more natural

Cons

• requires add support of options to <Link /> component

[Guria]

// cerebral-module-signal-merge-prev-payload-option
function (module, controller) {
  var store = {}

  controller.on('signalTrigger', function (event) {
    if (event.signal.options.merge) {
      var prevPayload = store[event.signal.signalName] || {}
      store[event.signal.signalName] = event.signal.input = Object.assign(prevPayload, event.signal.input)
      delete event.signal.options.merge
    }
  })

  module.addServices({
    dropStore: function () { store = {} }
  })
}

Pros

• could work without router
• Link support as in previous example (with options prop)
• should work now (theoretically)
• could be used as module

Cons

• could break in future:
  • depends on Remove special signal options hard codes cerebral/cerebral#213 (comment) which hides options from signalTrigger event
  • mutating initial not guaranteed to work and could be changed

[Guria]

// cerebral-module-signal-merge-prev-payload-option
function (module, controller) {
  var store = {}

  controller.defaultOptions.context.push(function (context, execution) {
    if (execution.signal.options.merge) {
      var prevPayload = store[event.signal.signalName] || {}
      store[event.signal.signalName] = event.signal.input = Object.assign(prevPayload, event.signal.input)
    }
  })

  module.addServices({
    dropStore: function () { store = {} }
  })
}

Pros

• same is in previous example
• doesn't mutate input in event handler

Cons

• could break in future:
  • depends on Remove special signal options hard codes cerebral/cerebral#213 (comment) which hides options from signalTrigger event
• can't not work now:
  • depends on context providers proposals: Modules, lets talk context cerebral/cerebral#204 (comment) and Modules, lets talk context cerebral/cerebral#204 (comment)

[christianalfoni]

Hi @Guria

I think this is the cleanest implementation:

<Link signal={someGridOpened} params={{ page: 1 }} options={{ merge: true }}>
  Next Page
</Link>

I think it is wise to keep the router as one module for now and keep using these options. I think it is a simple concept that you can see all over. Creating a single signal, general options for all signals, options passed on Link etc. It is the same "options object".

There is still a discussion on how these options should be passed to the debugger. Eyh... maybe we should just:

controller.on('anyEvent', ({signal, options, [action]) {

}); 

So we pass the options as its own argument. That way also the debugger can just pick out any options it wants to pass it to the debugger. Yes... I think this is it! :-) What do you think?

[Guria]

cc @freele how are you feeling with this suggestions? does it solves your case?

[freele]

@Guria Yes, I think that it what was needed. As I see, both suggestions answer the initial request. Thanks for this.
The options is quite eloquent solution and may be expanded in future.