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

feat: Keep (Parent Route) Query Parameters #749

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

betocantu93
Copy link

This PR implements #476

Still needs tests, but wanted to get an opinion, thanks.

@betocantu93 betocantu93 changed the title feat feat: Keep (Parent Route) Query Parameters May 19, 2023
@betocantu93
Copy link
Author

known works now, not sure if its the best approach,
maybe reading path from
router._router.url insted of window.location.pathname could be better

@betocantu93
Copy link
Author

I think in order to make it more predictable:

  1. The mode can't change.
  2. The mode only dictates what get url () {} returns
  3. There will always be, on every link, all the modes via other getters

link.url => the mode will choose which
link.noneUrl => only the provided params
link.knownUrl => params of the current route when initialized + provided params
link.allUrl => all parent params + provided params
link.trackedKnownUrl => params of the current route + provided params (live)
link.trackedAllUrl => all parent params + provided params (live)

The mode can't change because if the getter for url consume tracked state because for example you change from known to tracked-known the link, it will now foreever be autotracked because tracked state was consumed

@betocantu93
Copy link
Author

betocantu93 commented May 19, 2023

Screen Shot 2023-05-19 at 17 24 25 Screen Shot 2023-05-19 at 17 22 40 Screen Shot 2023-05-19 at 17 31 53

@betocantu93 betocantu93 mentioned this pull request Jun 6, 2023
9 tasks
@gossi
Copy link
Collaborator

gossi commented Jun 6, 2023

I'm not sure if I fully understand what you are doing 🙈 but from what I grasp is to make a link that changes its query params, based on some tracked properties.

If so, that's what resources are for, no? Here is an example:

import { resource, resourceFactory } from 'ember-resources';
import { Link, link } from 'ember-link';

const LinkWithMode = resourceFactory((link, mode) => {
  return resource(({ owner }) => {

    Object.defineProperty(link, 'url', {
      get() {
        switch (mode) {
          // your logic here
        }
      }
    }

    return link;
  });
});

<template>
  {{#let (LinkWithMode (link "...") "your-mode") as |link|}}
    <a href={{link.url}}>...</a>
  {{/let}}
</template>

Overwriting the url() property with Object.defineProperty is well... a demonstration how to wrap that in a resource (that might be worth to look if this should receive a better API then).

What might be an alternative idea is to use a link builder as in #669 - or a combination of that:

import { resource, resourceFactory } from 'ember-resources';
import { Link, link } from 'ember-link';

const LinkWithMode = resourceFactory((buildLink, params, mode) => {
  return resource(({ owner }) => {

    const router = owner.lookup('service:router');

	// find qp from parent

	switch (mode) {
     // filter qp here and modify params (or make new ones)
    }

    return buildLink(params);
  });
});

<template>
  {{#let (LinkWithMode (link "...") "your-mode") as |link|}}
    <a href={{link.url}}>...</a>
  {{/let}}
</template>

// Update: I've only ever seen the linked issue afterwards

@buschtoens any idea to make it part of ember-link or provide an API to make that work?

@betocantu93
Copy link
Author

Sorry @gossi for the missing context, the issue described by buschtoens might better solve your questions #476

We need the tracked-all approach for most of our use cases, keeping parent qps in sync, like the LinkTo component does.

tracked-all:
All QPs of the current location are kept.
When the the current route / QPs change, the url will be updated live.
QPs provided via @query override these QPs individually.
A null / undefiend value in the @query object will remove a kept QP.

I like the resource and composition approach and it could work too, but IMHO I think the primitive itself should be able to and normal getters could do it. Also theres a demo on the dummy app.

Thanks for reviewing, I gladly to implement changes or rewrites

@gossi
Copy link
Collaborator

gossi commented Jun 7, 2023

I found the context later 🙈 I'm never using QPs, because they are deadly to me, so I avoid them 😂

I see the original issue is by @buschtoens itself. We were chatting on how to evolve ember-link into v3. As we want to remove UILink in favor of only Link, the idea is to have a behavior property, in which this functionality could be located.

I don't wanna make a say whether this should be part of ember-link or userland, I'll leave this to @buschtoens as he was knows the history of this package and it was his original idea.

I can say, if this will go into ember-link, I'm trying to fix CI over at #761 and this would be targeting v3, configured under behavior property.

@gossi
Copy link
Collaborator

gossi commented Jul 24, 2023

As Jan didn't respond here. The idea is this:

  • If it needs to be handled by ember-link (internally), please continue with the PR and I think, the best would be to give it another behavior
  • If this can be handled in userland instead with a customization then this is better suitable as its own addon

@betocantu93
Copy link
Author

Perfect @gossi we are already using it in prod, I will find some time this weekend to rebase, polish and add tests, I do think it's probably best if it's an internal thing.

My only doubts are around the private apis it uses. But I think(?) they are safe for now.

@gossi
Copy link
Collaborator

gossi commented Jul 26, 2023

Which APIs are you using? Might make sense to check their stability and consider making them public and adding your idea to the contributions section of the guides

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.

2 participants