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

[WIP] Preserve queryParams #47

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cibernox
Copy link
Contributor

So far I've proven that this is doable by using the -routing service and observing changes in the state of that service.

Questions to be answered:

  1. That service is not yet public. Ask the oracle (@rwjblue) wether or not this is a terrible practice
  2. Measure performance penalty. If this brings this addon in the like of performance with a regular {{link-to}} that would defeat the purpose of using it.

@cibernox
Copy link
Contributor Author

@GavinJoyce May I take some advice on how to measure the perf of this change? I imagine that setting an observer won't affect performance that much (although all helpers will recompute when qps change while now it doesn't, but that is a bug to me)

@GavinJoyce
Copy link
Contributor

@cibernox I just manually copy some code into a custom benchmark in a throwaway ember-performance branch and compare the results. I'm happy to do this later this evening if you want?

Thanks for doing this, this is a nasty bug and it's great that it'll be fixed

@cibernox
Copy link
Contributor Author

cibernox commented Jun 23, 2016

@GavinJoyce That would be great. That said, don't merge it yet. I need to test more edge cases.

  • Transition from /foo/bar?one=1&two=2 with {{href-to 'foo.qux'}}, where one is a qp that belongs to foo and two is a qp that belongs to bar but not to qux. Expected: one=1 is preserved, two=2 dissapears.
  • Transition from /foo/bar?one=1&two=2 with {{href-to 'foo.qux'}}, where one is a qp that belongs to foo and two is a qp that belongs to bar and qux. Expected: Both remain.
  • Transition from /foo/bar?one=1&two=2 with {{href-to 'foo.qux'}}, where one and two are qps that belongs to foo. Expected: Both remain.
  • Transition from /foo/bar?one=1&two=2 with {{href-to 'foo.qux'}}, where one and two are qps that belongs to bar and qux. Expected: Unsure.
  • Transition from /foo/bar?one=1&two=2 with {{href-to 'another'}}, where one and two are qps that belongs to foo and another. Expected: None is preserved (I think).

@GavinJoyce
Copy link
Contributor

GavinJoyce commented Jun 23, 2016

@cibernox I didn't manage to get benchmark data, I was planning to modify GavinJoyce/ember-performance#1 but ember-performance has changed significantly since then.

onQpsChange: Em.observer('_routing.currentState', function() {
  this.recompute();
}),

This seems like it will be expensive on any route transition, every href-to will recompute. I wonder can we only recompute if a query param has changed? Or, perhaps we could skip the recompute if there are no query params?

@cibernox
Copy link
Contributor Author

cibernox commented Jul 1, 2016

I've been able to create a couple more complex scenarios that still fail. Over the weekend I'll try to figure out how to better do this.

I've also seen on the core notes that they agreed to proceed with your url-for RFC, so I might ping someone to give some guidance on this.

The basic idea is that when a link doesn’t specify the value for a query
param, the current value is preserved. 

So, given the hierarchy

```
qps // defines a QP named “string”
  qps.details // defines a QP named “nestedString”
    qps.details.more // defines a QP named “doubleNestedString”
```

Links inside `qps.details.more` that point to `qps.details` will contain
“nestedString=bar&string=foo”, but never “doubleNestedString”.
Links inside `qps.details` that point to `qps` will contain “string=foo”,
but never “nestedString” or “doubleNestedString”.
Links inside `qps.details` that point to `qps.details` with a different
will model will contain “string=foo” but never “nestedString” or “doubleNestedString”.
Links from a route to any descendant route will always include all the 
query params defined in the path.
@ilucin
Copy link
Contributor

ilucin commented Feb 22, 2017

I was experimenting with this because I need it on my own project. Few observations:

  • observer on _routing.currentState fires even though routerJsState.fullQueryParams did not change. I've fixed it with my proxy routing service that abstracts currentQueryParams away.
  • fullQueryParams sometimes contained old value so I'm using routerJsState.queryParams instead. Not sure what's the difference between those two.
  • It only makes sense to preserve query params that belong to parent route handlers of the route being passed to href-to.
  • I'd maybe rather opt-in into this feature on every {{href-to}} than have it on by default because of performance penalty on route switching. On my (highly specific) use case this performance penalty is significant.

@scalvert
Copy link

Is it worth investigating this with the new public router service?

@cibernox
Copy link
Contributor Author

@scalvert I believe it's not. I mean, its probably worth pursuing, but the approach would be totally different from different. Tests seem valuable tho

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.

4 participants