veliovgroup / flow-router

🚦 Carefully extended flow-router for Meteor
https://packosphere.com/ostrio/flow-router-extra
BSD 3-Clause "New" or "Revised" License
202 stars 29 forks source link

Variable & optional prefix group #61

Closed arichter83 closed 5 years ago

arichter83 commented 5 years ago

I have optional partners in my scenario which get a specific site, but all the routes stay the same. E.g.:

/ => homepage no partner
/create => subpage no partner
/one/ => homepage partner one
/one/create => subpage partner one
etc.

The list of partners is limited and will not collide with the subpages, so I created it like this:

var group = FlowRouter.group({
  prefix: "/:partner(one|two|three)?"
})
group.route("/", {name: "base", ...)
group.route("/create", {name: "create", ...)

For this the pipe | is missing in the Router.pathRegExp and I created a PR https://github.com/VeliovGroup/flow-router/pull/60

Also I was thinking of a method to make this partner "sticky" for all further FlowRouter.go(), etc. calls and wondered if I am missing an option in the existing code. My implementation would be like the following:

  sticky_field = 'partner'
  path(_pathDef, fields = {}, queryParams) {
    // Keep a sticky field
    if(this.sticky_field && !fields[this.sticky_field]
        && this._current.params && this._current.params[this.sticky_field]) {
      fields[this.sticky_field] = this._current.params[this.sticky_field]
    }

Do you think that a similar implementation would be of value to others?

dr-dimitru commented 5 years ago

Hello @arichter83 ,

  1. I your case it seem to use more logical approach making "partner" as subpath /:partner/:partnerName?
  2. pathRegExp is editable by default — see docs, and addressed in #25, asn solver in #28 by @nicooprat

Although I've merged your PR, now thinking — are we really want to publish such changes? Waiting for your opinion @arichter83

arichter83 commented 5 years ago

Dear @dr-dimitru,

the PR was just the | and I think it is optional to merge and no need to publish. I didn't know about the overriding!

And in regards to the sticky_field, that was just a draft. But /:partner/:partnerName? is no solution - /:partner(one|two|three)? means /, /one, /two or /three.

dr-dimitru commented 5 years ago

Okay, going to revert latest merge. Please, feel free to use pathRegExp to meet your needs 😉

Feel free to reopen it in case if the issue is still persists on your end.