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

@route argument #634

Closed
Gaurav0 opened this issue May 29, 2020 · 20 comments
Closed

@route argument #634

Gaurav0 opened this issue May 29, 2020 · 20 comments

Comments

@Gaurav0
Copy link
Contributor

Gaurav0 commented May 29, 2020

The @model argument that was added to route templates got me thinking... what if we could pass our own arguments from a route to a template that would also be read only?

Example:

// app/routes/my-route.js
export default MyRoute extends Route {
  @tracked trackedArg = 'tracked argument';

  get trackedProp() {
    return this.trackedArg + ' property';
  }

  model() {
    return ...
  }

  @action
  myAction() {
    // do something
  }

  routeArgument() {
    const { trackedArg, trackedProp, myAction } = this;
    return { trackedArg, trackedProp, myAction };
  }
}
<!-- app/templates/my-route.hbs -->
<SomeComponent
  @myModel=@model
  @someArg=@route.trackedArg
  @someOtherArg=@route.trackedProp
  @someAction=@route.myAction
/> 

Basically the routeArgument hook would return an object that would be represented as @route on the template. This could greatly reduce our reliance on controllers at the top level. By freezing the object returned by routeArgument, we could enforce one way binding. Our route templates could be much more like template only components.

What do people think?

cc/ @NullVoxPopuli @pzuraq @rwjblue

@gossi
Copy link

gossi commented May 30, 2020

It works partly with @model today. See here: https://ember-twiddle.com/c965eb18b7f6938726fd5333caa87d4e?openFiles=templates.application%5C.hbs%2C The template doesn't get the tracked value here, yet logging it works then (keep console open).

This idea is navigate around current route implementation and make that one a little bit easier.

My current understanding of the whole topic is, that routing needs to be re-thought within ember, at the moment it is heavily overloaded to what most people use it for. Routes have loading and error states, while the trend is more towards provider components or services (eg. ember-await for example).

I think routing should be reduced to what is required to result in a resource given the current URL, which are two parts: URL segments and query params. Those are available to routes as arguments, controllers will be eliminated, the hooks of routes are reduced, too (maybe activate/deactivate is enough - or constructor()/willDestroy() 😉). A route class will behave similarly to glimmer components.

On the other hand the routing layer should enable such experiments that @machty is running =)

That's my current state of mind 😂

@rwjblue
Copy link
Member

rwjblue commented Jun 5, 2020

IMHO, we should go a slightly different direction here. We shouldn't expose the route directly to templates, we should add a hook that represents the named arguments to be passed into the template. I'm using the name args here, but basically I'm suggesting:

export default class extends Route {
  model() {
  }

  async args() {
    // pseudo code
    return {
      model: await this.model(params, transition),
      someOtherThing: this.whateverHere
    }
  }

  @action
  whateverHere() {
    // zomg
  }
}

And that hook, returns a promise that resolves to an object whose keys are the full list of named arguments available in the template. So in the above snippet, you would be able to write {{@someOtherThing}} in the template, and have it refer to the routes whateverHere method.

@locks
Copy link
Contributor

locks commented Jun 5, 2020

I think exposing the route instance like that is pushing users in the wrong direction, and well as having tracked and computed state in the route. I think we would increase the possibility of mis-use and confusion.
The direction proposed by @rwjblue seems more aligned with the direction of the framework and an incremental step on @model.

@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Jun 5, 2020

IMHO, we should go a slightly different direction here. We shouldn't expose the route directly to templates, we should add a hook that represents the named arguments to be passed into the template.

I'm not proposing exposing the route directly to templates. I'm proposing a routeArgument hook that returns an object that will be made read only before being exposed to the template. Please reread the proposal.

@rwjblue
Copy link
Member

rwjblue commented Jun 7, 2020

@Gaurav0 I did not understand that nuance on first read through. I think what you said and what I said are very close. The differences (as far as I can tell) are that my proposal:

  • allows arbitrary named arguments to be used in the route template
  • Explains / justifies / rationalizes / whatever how @model ends up being a thing
  • Easily provides a way to name your model (which IMHO was a large downside of {{@model}} initially)

@jrjohnson
Copy link

I love the idea of being able to pass named arguments into the route template. I'm wondering if we can get rid of the ceremony and use a decorator:

// app/routes/my-route.js
export default MyRoute extends Route {
  @arg @tracked trackedArg = 'tracked argument';

  @arg
  get trackedProp() {
    return this.trackedArg + ' property';
  }

  model() {
    return ...
  }

  @action
  @arg 
  myAction() {
    // do something
  }
}

@arg is probably a bad name for this, but some way to decorate things that will show up in the template. the model() hook would be implicitly assigned this decorator. If it is a promise it could be resolved first (or not).

@rwjblue
Copy link
Member

rwjblue commented Jun 7, 2020

@jrjohnson - I think we probably could do something like that, but I personally think it makes sense to start with underlying mental model / primitive (e.g. the method you can implement). At that point, it would be pretty easy to make an addon that would provide the decorator like in your snippet (gathering the decorator implemenations and implementing the hook).

@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Jun 8, 2020

@Gaurav0 I did not understand that nuance on first read through. I think what you said and what I said are very close. The differences (as far as I can tell) are that my proposal:

  • allows arbitrary named arguments to be used in the route template
  • Explains / justifies / rationalizes / whatever how @model ends up being a thing
  • Easily provides a way to name your model (which IMHO was a large downside of {{@model}} initially)

@rwjblue My only concern is that it potentially breaks @model and prevents any future special @ things in the route template.

@rwjblue
Copy link
Member

rwjblue commented Jun 8, 2020

My only concern is that it potentially breaks @model

Nah, I don't really think this is likely to be an issue. If you don't want @model at all (in my proposal) it won't be surprising to use @post or whatever other name. Also, by default, I'd expect folks to continue to use @model for the most part.

prevents any future special @ things in the route template

When named arguments were introduced, we intentionally reserved a very large namespace that can be used for special purposes (anything starting with a capital letter, for example).


I generally think less unexplainable "magic" is better, and having this turn out to be something fairly easy to remember is good IMHO.

@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Jun 8, 2020

Ok. How about this?

export default class extends Route {
  model() {
  }

  get args() {
    return {
      model: this.modelFor(this.routeName),
      whateverHere
    };
  }

  @action
  whateverHere() {
    // zomg
  }
}

This should avoid triggering the model hook twice. It also makes it clear that args updates just like any computed property and is a read only property.

Also what should happen if model is not supplied in returned object from args? What should happen if args hook is not implemented?

@rwjblue
Copy link
Member

rwjblue commented Jun 9, 2020

I don’t particularly think we should further modelFor usage. I don’t want the router to have to maintain its existing async hook behavior and to introduce another async hook between the model* running and attempting to render templates.

To clarify my original snippet a bit, I think we would need some massaging to properly call beforeModel, model, afterModel (likely in the super implementation).

@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Jun 9, 2020

I don’t particularly think we should further modelFor usage. I don’t want the router to have to maintain its existing async hook behavior and to introduce another async hook between the model* running and attempting to render templates.

I don't understand. I'm trying to avoid making args an async hook and reimplementing the router's current behavior.

modelFor is public api. Why should we avoid it?

To clarify my original snippet a bit, I think we would need some massaging to properly call beforeModel, model, afterModel (likely in the super implementation).

This seems to go down the road of calling everything twice?

@rwjblue
Copy link
Member

rwjblue commented Jun 9, 2020

I'm trying to avoid making args an async hook

I see. That is not my goal. My goal is to rationalize the current async behaviors in terms of a broader system than just beforeModel/model/afterModel.

modelFor is public api. Why should we avoid it?

In this case, because it doesn’t help make the proposal simpler. In the general case, because it is conceptually very bizarre to exist in the first place. IMHO, it’s primary issue is that it’s attempting to provide sync access to async data.

This seems to go down the road of calling everything twice?

How so?

@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Jun 9, 2020

This seems to go down the road of calling everything twice?

How so?

I wasn't trying to reimagine the entire Ember Route API but to make a focused RFC to solve the issue of requiring controllers to solve some simple issues (mostly because I know that some people will resist having to move code into controllers with RFC #632 and I want to provide an escape hatch.)

If we don't change the whole Route api, just adding the args hook and the code you presented will cause Ember to call the *model hooks as it already does, and then you call them in the args hook again.

@chancancode
Copy link
Member

We are, in fact, trying to reimagine the route API, as per the roadmap, both to improve accessibility and to make things fit better into the Octane model, so I think a little broader perspective is warranted at this time.

A lot thoughts have been given in this area and in the rough API design @rwjblue proposed in the past already. The first time we discussed that was at least in the 2017 face-to-face.

Roughly, the "end goal" (of the particular proposal) is to transition into into this:

class Route {
  async beforeModel(transition) { ... }
  async model(params, transition) { ... }
  async afterModel(model, transition) { ... }
}

to this:

class Route {
  // TODO: do we still need before/after hooks? seems like not but TBD
  async args(params, transition) { ... }
}

As a transition path, you can imagine (mega hand wave) that Ember adds this to the super class:

class Route {
  async args(params, transition) {
    if (typeof this.beforeModel === 'function') {
      // TODO: deprecate or not?
      await this.beforeModel(transition);
    }

    let model;

    if (typeof this.model === 'function') {
      // TODO: deprecate or not?
      model = await this.model(params, transition);
    }

    if (typeof this.afterModel === 'function') {
      // TODO: deprecate or not?
      await this.afterModel(model, transition);
    }

    return { model };
  }
}

...and that Ember will stop calling the other hooks directly, just through the super.args() implementation (if super is called). Or perhaps for practical reasons this logic has to be in the framework that's calling the code instead of in the args implementation, closer to @rwjblue's original snippet:

class Route {
  async args(params, transition) {
    if (typeof this.model === 'function') {
      // TODO: deprecate or not?
      return { model: await this.model(params, transition) };
    }
  }
}

class Router {
  private async transitionIntoRoute(route, params, transition) {
    if (typeof route.beforeModel === 'function') {
      // TODO: deprecate or not?
      await route.beforeModel(transition);
    }

    let args = await route.args(params, transition);

    if (typeof route.afterModel === 'function') {
      // TODO: deprecate or not?
      await this.afterModel(model, transition);
    }

    return args;
  }
}

TBD. Lots of details to figure out.

Anyway, I think that's what @rwjblue is is getting it and what the "rough sketch" of the args hook API entails. I don't think anyone is trying to call everything twice. That would be both slow and incorrect (causes problem existing code didn't account for).

One way or the other, the end goal is to transition away from the single model argument into allow passing arbitrary named arguments. That gets you probably 70-80% there of "routable components" and not needing controllers. It essentially turns the model into "route invokes a template-only component with args". It may even be good enough that we can deprecate controllers once this lands and when we have an alternative place for QPs to live.

@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Jun 9, 2020

Sorry, I was not privy to past conversations regarding this topic. The roadmap is rather vague and did not mention a new Route api.

Now my concern re: #632 is that we are deprecating send with string based actions in favor of moving actions to controllers in some situations. Some people will resist doing this because rightly or wrongly, they resist putting code in controllers. Is there anything we can do in the short term?

@Gaurav0
Copy link
Contributor Author

Gaurav0 commented Jun 18, 2020

@chancancode @rwjblue I've thought about it, and I've decided that I oppose explicit, overrideable routing or model resolution. Routing and model resolution that "just works" is a strength of Ember.js, not a weakness, and my quick non scientific survey indicates the community neither cares about or wants explicit routing or model resolution.

I would not like to be able to write code that breaks nested model promises, for example, by accident.

@rwjblue
Copy link
Member

rwjblue commented Jun 22, 2020

I would not like to be able to write code that breaks nested model promises, for example, by accident.

Yep, totally agree, but I don't see how this ties into the prior conversation at all.

@mehulkar
Copy link
Contributor

mehulkar commented Jul 7, 2020

fwiw, @chancancode's example is basically how it works now. I recently wanted to move this underlying resolve() function up to the Route class level also, so that I could override it and attach a callback to the returned promise.

https://github.com/tildeio/router.js/blob/v6.2.5/lib/router/route-info.ts#L222-L234

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

I'm closing this due to inactivity. This doesn't mean that the idea presented here is invalid, but that, unfortunately, nobody has taken the effort to spearhead it and bring it to completion. Please feel free to advocate for it if you believe that this is still worth pursuing. Thanks!

@wagenet wagenet closed this as completed Jul 23, 2022
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

No branches or pull requests

8 participants