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

Routing Improvements #1279

Closed
4 tasks done
Slashek opened this issue Oct 28, 2020 · 41 comments
Closed
4 tasks done

Routing Improvements #1279

Slashek opened this issue Oct 28, 2020 · 41 comments

Comments

@Slashek
Copy link
Contributor

Slashek commented Oct 28, 2020

  • - As a developer, I would like to be able to use named parameters in the slug

for example, defining slug products/:id/reviews/:review_id should be able to match URL products/1/reviews/2 and automatically populate context.params.id with 1 and context.params.review_id with 2

  • - As a developer, when I define slug my_page it should raise 404 for URL /my_page/something

  • - As a developer, I should be able to use anywhere in the slug

  • - As a developer, I would like to point all request to one page

@mattwalter91
Copy link

Please allow . in URLs too, such as /index.php/my_page

This will allow us to build redirects from other systems to new pages on pOS.

@richxrich
Copy link

Please allow . in URLs too, such as /index.php/my_page

This will allow us to build redirects from other systems to new pages on pOS.

That was already fixed about a month ago

@richxrich
Copy link

  • - As a developer, I would like to be able to use named parameters in the slug

for example, defining slug products/:id/reviews/:review_id should be able to match URL products/1/reviews/2 and automatically populate context.params.id with 1 and context.params.review_id with 2

  • - As a developer, when I define slug my_page it should raise 404 for URL /my_page/something
  • - As a developer, I should be able to use anywhere in the slug

Control over the first level slug, currently we can not control test in website.com/test - it would allow for full dynamic routing and a basic htaccess level of control (at least for routing)

@mattwalter91
Copy link

Please allow . in URLs too, such as /index.php/my_page
This will allow us to build redirects from other systems to new pages on pOS.

That was already fixed about a month ago

It throws a 404 for me, so maybe it re-broke? -> https://www.dropbox.com/s/q21aup727xx7bq8

@richxrich
Copy link

Works for me on staging, the latest version of pos-cli doesn't have the editor anymore so I couldn't compare apple for apples.

@onecreative
Copy link
Contributor

Moving this here from ClickUp.

I brought this up ages ago, and I still think it's a good idea. One of the messiest experiences I have in pOS is working with extract_url_params. The problem isn't the feature itself though. It's the pattern of creating multiple dynamic pages in one file. The current solution for the mess is to break up the different "pages" via includes. But that is still pretty messy, and the logic in the actual page gets complex if you have more than a level or two of depth.

The linked doc is a good start to a spec for this. I think this would become a favorite feature for a lot of people.
https://app.lucidchart.com/invitations/accept/666acf7e-a6c4-4005-b295-b162efbf3286

There are some updates I would make to this spec that I think would make it a little more flexible. For instance, I wouldn't:

  • Alter model_schemas to have parent child relationships
  • Dynamically filter records based on a parent page.

Instead, I would:

  • Utilize the parent property of the page to simply expose the parent page's yaml properties, including the variable in the slug. Then let the dev use that information to do their own filtering on the page.

@Slashek
Copy link
Contributor Author

Slashek commented Oct 30, 2020

Works for me on staging, the latest version of pos-cli doesn't have the editor anymore so I couldn't compare apple for apples.

Matt issue is the dot in the middle of the path, not in its last part - /test/hello.world works fine, however /hello.world/test does not :)

@onecreative if you had an equivalent of the extract_url_params built in the routing as in the first example (slug products/:id/reviews/:review_id should be able to match URL products/1/reviews/2 and automatically populate context.params.id with 1 and context.params.review_id with 2) , then hopefully it will be one of the more pleasant experiences!
Could you please write more about the parent property in this context, as I am missing something

@Slashek
Copy link
Contributor Author

Slashek commented Nov 5, 2020

Progress is going well for this one - it seems we should be able to start QA-ing some of the routing improvements at the end of the next week. The first iteration should ensure the new system is in place with full backward compatibility, as well as the possibility to use named parameters, including the possibility of creating a Page to which all requests will be routed.

@richxrich
Copy link

possibility of creating a Page to which all requests will be routed.

All requests? Including assets and media? Is it for a site is down message or something?

@Slashek
Copy link
Contributor Author

Slashek commented Nov 5, 2020

possibility of creating a Page to which all requests will be routed.

All requests? Including assets and media? Is it for a site is down the message or something?

@richxrich assets/media served through CDN of course not. Sometimes you might want to handle routes via JS. The maintenance page is another use case, which would require a bit slightly different treatment because the way this Page * is intended to work is as a fallback - i.e. there's no other page which can be matched to the request URL. However, the possibility to overwrite and hardcode the priority of the page will be doable in the future.

@richxrich
Copy link

@Slashek so its for a single file application website? Maybe I'll wait until I see it 👍

@Slashek
Copy link
Contributor Author

Slashek commented Nov 6, 2020

@richxrich yes, it's one of the use cases!

@mattwalter91 the possibility of having . in the middle of the URL will be supported as well!

@richxrich
Copy link

I'm keen for total routing, but transparently. Will we have to have a visual static first slug or is it a behind the scenes thing?

Not looking for the PHP methods of

/module.php?file=/something/somethingelse

or something in POS where "module" or whatever had in the URL, would mean the entire site would have to run in second level slug

/module/something/somethingelse

Is it possible to route everything to the "module" file (in this example) and also run the site directly from slug1?

i.e.

/something/somethingelse

with "module.liquid" as the route processor (just making up names here)

@Slashek
Copy link
Contributor Author

Slashek commented Nov 9, 2020

@richxrich one will be able to create a slug :path in which scenario both /something and /anything will be matched by it - this does not differ much from `*path. Initially, for backward compatibility, we would need to automatically expand created based on max_deep_level property [ which by default is 3 ], but we will be looking into giving an option to remove this behavior.

What I mean by expanding is that currently if you create a page with slug abc, then all three URLs will be matched - /abc, /abc/1 and /abc/1/2. Unfortunately, most of the sites rely on this unintuitive behavior. The new routing will be much more precise, however, we will have to think about how to leverage this. Any feedback is welcomed.

@onecreative
Copy link
Contributor

@Slashek, I'd like to see this go one step further by introducing parent and name to page yaml. This would allow us to split up complex multi-page logic into multiple files. The concept is that each page's slug is relative to its parent page. If no parent is specified, it's assumed that the parent is the root url.

For example, instead of having one page with
slug: business/:business_id(/members/:member_id)
and then filling that file with the logic for potentially four levels of pages, you would have two files:

name: business
slug: business/:business_id

and

name: business_member
slug: members/:member_id
parent: business

As the child page, the business_member slug would be relative to the parent, making it effectively business/:business_id/members/:member_id

As a child, the yaml from the parent would also be available for reference purposes.

@onecreative
Copy link
Contributor

onecreative commented Nov 10, 2020

I like the idea of the parenthesis making part of the slug optional. (i.e., business(/:business_id)). So in keeping with that, I think if two pages are set up as parent/child, then any optional slug segments on the parent would have to be present in the url before the child page comes into play. For example, with the relationship below:

name: business
slug: business(/:business_id)

and

name: business_member
slug: members(/:member_id)
parent: business

The child page would always equate to business/:business_id/members... and never business/members...

@richxrich
Copy link

@onecreative : For parent, you are assuming that all sub pages (children) have the same parent / URL structure. Case example, products listed in multiple categories will have different parents / urls.

For parent, clientside you could use go back

This assumes the page knows it has a parent (a little hacky as well)

But I'm guessing you want to do this for breadcrumbs on a page. If the page doesn't know if it has a parent or not:

{%- assign slugs = context.location.pathname | split: "/" -%}
{%- assign current = slugs | size | minus: 1 -%}
{%- assign parent = current | minus: 1 -%}
{% if current == 0 %}
  I have no children
{% else %}
  I have children and they look like:
  Child (this page): {{ slugs[current] }}
  Parent: {{ slugs[parent] }}
{% endif %}

Example could be cleaned up and you can then loop it in a nice function and build dynamic breadcrumbs based on the URL structure.

@onecreative
Copy link
Contributor

onecreative commented Nov 11, 2020

There are a lot of ways to create page relationships. I doubt the parent/child model I explained will meet all of those needs. Its main purpose is to reduce the complexity of pages that would otherwise have multi-page logic in them. Personally, I don't like having a ton of partials that I treat as "pages".

@onecreative
Copy link
Contributor

onecreative commented Nov 11, 2020

This assumes the page knows it has a parent

@richxrich The relationship is defined on the child page's yaml, so the page would definitely know it has a parent.

@onecreative
Copy link
Contributor

For parent, you are assuming that all sub pages (children) have the same parent / URL structure. Case example, products listed in multiple categories will have different parents / urls.

Also, @richxrich, the page content is decided by you, not the url. This pattern will work for multi-category product catalogs just as easily as the one I explained. The child page has access to the parent page yaml data, so your logic can use that pattern however you need.

In the end, it's just a pattern like we are already successfully using with url_params. This is just easier to manage.

@richxrich
Copy link

@onecreative Not saying it's a bad idea for some use cases, just we would not use it all because we don't have linear parents and children.

@onecreative
Copy link
Contributor

@richxrich That's cool. No feature will meet every need.

I'm curious, though. Can you give me an example parent/child relationship that wouldn't work with it?

@Slashek
Copy link
Contributor Author

Slashek commented Nov 11, 2020

Currently, the page is an all-in-one thing - it's routing, it's a controller, it's also a view. What we are thinking about as part of the new routine is to remove the routing capability of the Page by extracting routing to a dedicated file. We don't know yet exactly how it would look, but if you think of Page and Partial, the only difference is that Page has built-in routing. This also seems like a good way of getting rid of an intuitive behavior of slug and max_deep_level.

@onecreative
Copy link
Contributor

Sounds like a steeper learning curve and less intuitive user experience.

@onecreative
Copy link
Contributor

@Slashek Whatever happens here, it shouldn't steepen the learning curve. Right? This doesn't sound like a good direction. Can you sell me on it?

@Slashek
Copy link
Contributor Author

Slashek commented Nov 12, 2020

Just a note that the new routing engine has been deployed to staging.

@onecreative it simplifies the directory structure [ no need for app/views/pages and app/views/partials ], as in the routing file you will be able to say which template you would like to render as which route - it will be easy to get an overview of the application thanks to this.

Additionally, when you create modules, it's not clear where "Page" belongs - because the rendering part most of the time should belong to the module, however, the routing usually belongs to the application - but right now it's combined.

Later on, it should also allow for easier namespace management, I imagine authorization policies could be easily applied to the whole namespace [ which you probably referred to as parent in your suggestion ] instead of each page individually, which makes it easier for a mistake like forgetting adding auth policy when you create a new admin page, etc.

@richxrich
Copy link

@Slashek What is the roadmap plan for "where the page belongs" - Are you thinking of pushing things into app or keeping it combined?

@onecreative
Copy link
Contributor

@Slashek I don't see the vision like you do, and I've never seen it in action. I just hope this doesn't make pOS more difficult. It sounds like a move that is driven by SPAs and backend coding conventions.

That said, if it makes its setup more logical and enables better control, it will be easier to teach even if it is different.

@pavelloz
Copy link
Contributor

pavelloz commented Nov 12, 2020

IMO

https://sapper.svelte.dev/docs#File_naming_rules > https://guides.rubyonrails.org/routing.html

@onecreative
Copy link
Contributor

@pavelloz Was that aimed at me?

@Slashek
Copy link
Contributor Author

Slashek commented Nov 16, 2020

So "svelte way" is pretty much what we already do - by default slug is determined based on the file path. With the new improvements, we will be also able to use named parameters and we can easily transform app/views/pages/my/[page].liquid to /my/:page. The things which are not clear at this stage is:

  1. How do we go about the request method? A potential improvement suggestion is that it should be possible to define one route for multiple request methods. Currently, this has to be one value.
  2. Do we need optional named parameters? The new engine is capable of defining the slug to be /search(/:country)(/:city), in which scenario all following URLs will be matched - /search, /search/Poland, /search/Poland/Warsaw.
  3. Do we want one Page to be matched against multiple formats and let developers control what should be returned for each format?
  4. Do we want to decouple the view part from the controller part, i.e. do not allow to add any rendering to the page and instead just specify which parts to render?

@richxrich
Copy link

  1. Regex patterns would be sweet,
  2. Yes, why remove them?
  3. Why not both?
  4. Yes, for routing only situtations (assets, redirects etc) where anything else would be overhead.

@onecreative
Copy link
Contributor

@Slashek I don't entirely know how it's related, but I keep feeling like we need to solve the lack of 301 management as part of this. Otherwise, I think we could end up getting stuck in a direction that won't be ideal for solving that problem.

@chrisdanek
Copy link
Contributor

There are a few big nuisances I have when working with PlatformOS:

  1. Working with nested routes, e.g. api/products for product index, api/products/:id for a single product, api/products/:id/versions for product versions. One has to handle all of that logic in one large file, parsing the context.params object. It gets increasingly complex the more specific route we want to render.
  2. Splitting application logic between pages and partials. Jumping between the two folders instead of having all of the related files in one place is cumbersome.
  3. Using the format property on the page unexpectedly changes the output URL by adding the format extension. It’s not apparent to the new developers why slug: foo; format: json renders nothing when I visit the slug I defined for my page.

I firmly believe that the new routing implementation could resolve all of these issues if done correctly. I am all for decoupling routing from page processing. Routes should point to defined partial files. There are several properties related to every route:

  • actual URL (with dynamic params)
  • request method - GET / POST
  • response status - 2xx, 3xx, 4xx, 5xx
  • response content-type / format
  • redirect URL

If we utilize YML, then route definition would look very similar to what we have in pages right now.

---
- url: api/products/:id
- redirect_to: /other-url
- response_status: 302
... etc
---

There is a matter of where to store these routes. We could keep each route in a separate file, or we could have one configuration file, storing routes for the whole application. I prefer the second approach, as I can easily see the entire URL schema in one place and adjust related routes easily.

I am having a hard time figuring out how to achieve all that with only file name based routes:

  • Using regex to validate URL parameters will be impossible due to file system restrictions (characters like * etc.)
  • Request methods
  • Defining redirects etc

Finally, I believe we can have our cake and eat it too. Moreover, I think we will be forced to do so to keep the current system backward compatible.

We could use one routing file, like routes.yml, pointing directly to one of the views in views/partials. Configuration in routes.yml would take precedence, with fallback to the current, file-based system. This approach would allow us to gradually migrate any of the current projects to the new system over time, whenever new routes are created or rewritten.

@onecreative
Copy link
Contributor

I like and agree with @chrisdanek 's comments.

And on the 301 issue I mentioned, I think we should have a special table for 301s that is managed exclusively via graphql. The system will look at the routes.yml definitions, but give priority to the 301 table when routing. But the 301s would have to account for custom parameters.

@richxrich
Copy link

I got frustated so created my own endpoints and process to handle everything... but can only do so much without routing

@onecreative
Copy link
Contributor

How would this work with search parameter naming conflicts? For instance:
slug: apis/listings/people(/:id)
url: apis/listings/people/45?id=4

Would the output be {id: 45} or {id: 4}?

@richxrich
Copy link

@onecreative wouldn't that be the same as

/test?id=12&id=54

Under a usual HTML server the form data would be id=12,54 but under POS the ID is the last element, in this case 54 (unless you process the headers manually).

I'm assuming the same here?

@onecreative
Copy link
Contributor

We could avoid the conflict completely if slugs were not accessed in the same place as search params.

@richxrich
Copy link

There isn't a conflict I don't think, same thing happens currently if you have a querystring variable and a form variable with the same name... it takes the later one (in my opinion it should be an array) but hey.

@Slashek
Copy link
Contributor Author

Slashek commented Dec 16, 2020

The engine is stable, will close this card as consider phase 1 done. For further improvements, we'll create a separate issue.

@Slashek Slashek closed this as completed Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants