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

named dynamic segments #382

Closed
wants to merge 2 commits into from

Conversation

luxzeitlos
Copy link

@Gaurav0
Copy link
Contributor

Gaurav0 commented Oct 4, 2018

Very nice for a first RFC. I like the way this eliminates the need for always run model hook.

You don't need to include the template questions, just the answers for each part.

@luxzeitlos
Copy link
Author

Thanks! I've deleted them.

@mehulkar
Copy link
Contributor

mehulkar commented Oct 7, 2018

👍 Looks great. I'd definitely like to be able to pass named params for dynamic segments. Couple questions:

  • Can this be implemented and still allow the new dynamic segments option to take models? I think there are still questions about whether or not passing models and skipping the model hook is worthwhile design and it seems like an overreach to include them here.
  • Can this RFC address the API for {{link-to}}? If this RFC moved forward without Router link component and routing helpers #339, it should have a story for how to pass named dynamic segments from templates.

(PS, I don't think you want to delete the template file, you might want to revert that)

@luxzeitlos
Copy link
Author

Can this be implemented and still allow the new dynamic segments option to take models? I think there are still questions about whether or not passing models and skipping the model hook is worthwhile design and it seems like an overreach to include them here.

We could from a technical perspective, but we would need a new serialize API for this. And actually this would make the scope of this RFC larger, not smaller, because this new serialize API needs to be part of it. RFC 0120 basically already proposes a different serialize API, which is used by engines so far.

If we want to introduce serialize we need to decide if we want to implement it on the route or only like the RFC 120 serialize API. And also how to be compatible to the old API.

However the current design of this RFC allows it totally to have a future RFC adding this functionality again if we really need it. Especially this is important:

dynamicSegments will not take model instances but only primitive datatypes.

This means we have the possibility to extend the API to take Objects in any way we wont without breaking anything using this RFC. As long we don't change functionality using primitive datatypes.


Can this RFC address the API for {{link-to}}? If this RFC moved forward without #339, it should have a story for how to pass named dynamic segments from templates.

I don't think it should. #339 is the path forward in my opinion. However I'l willing to reconsider if your opinion has larger support.


Currently I hope keeping this RFC very lean will allow it to merge sooner and easier to implement. However I also hope that it will be a foundation for good and consistent addons that we later can make standard with additional RFCs.

@bendemboski
Copy link
Collaborator

We could from a technical perspective, but we would need a new serialize API for this [still allowing models].

I'm not clear on why...currently you can pass either params or a model to transitionTo, and it calls serialize() if you passed a model, and doesn't if you passed params. So why couldn't this API also support passing either an id or a model and implement the same decision logic for calling serialize()?

I guess I'm missing why changing the API signature from positional arguments to named arguments forces a change in the underlying architecture.

@luxzeitlos
Copy link
Author

@bendemboski the current architecture maps array->hash, but new we would need hash->hash.
The current serialize API assumes the model is known. This will no longer be the case. Assume this call:

transitionTo('post.comment', { dynamicSegments: { blog, comment } });

With this routes:

this.route('post', { path: 'post/:post_id' }, function() {
  this.route('comment', { path: 'comment/:comment_id' });
});

I wont be able to know which model to pass to the serialize function of the post and which to the serialize function of the comment.

Don't get me wrong - its totally possible to extend the serialize API so this can be handled. Or another idea could be that in the path we specify the argument name passed to transitionTo. However all these things need discussion. And I wanted to make this RFC as small as possible. If we later decide we want to pass models or to cache models (we could do the first without the second) we can always add this with an additional RFC. Also multiple RFCs could discuss multiple approaches.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

Given the inactivity here I'm going to go ahead and close this. I apologize for the fact that this stagnated and if you would still like to try to move this forward, let me know and I'll ensure that it gets a proper review.

It's also worth noting that there is some new exploratory work being done around the router for Polaris. That may end up addressing this.

@wagenet wagenet closed this 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

Successfully merging this pull request may close these issues.

5 participants