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

Discussion: a better routing schema #76

Open
Jackymancs4 opened this issue Apr 13, 2018 · 8 comments
Open

Discussion: a better routing schema #76

Jackymancs4 opened this issue Apr 13, 2018 · 8 comments

Comments

@Jackymancs4
Copy link
Contributor

Jackymancs4 commented Apr 13, 2018

cc @rylnd

This was the last issue I had in mind to do, I promise!

Right now the endpoint set look like this:

/myAvatars/:id
/myAvatars/:size/:id
/myAvatars/face/:eyes/:nose/:mouth/:color
/myAvatars/face/:eyes/:nose/:mouth/:color/:size
/myAvatars/list

and as a developer I found it a bit confusing and hard coded.
So after some experiment I would propose you to implement the whole thing in a different way that (assuming #74 will land) should follow this schema:

root scope optional size params
avatars face * :id
avatars face :size :id
avatars face * :eyes/:nose/:mouth/:color
avatars face :size :eyes/:nose/:mouth/:color
avatars meta * list
avatars meta :size list
avatars meta * random
avatars meta :size random

I tried this new routing system on my local setup and it feels pretty good, being way more predictable and scalable. Also it doesn't add almost any logic on top of what exists now.

If you are wondering about \avatars\meta\:size\list I was thinking about letting list return an object like

{
      face: {...},
      size: :size
}

This add the ability to programmatically get the default size (or any size), that is another thing I find pretty useful.

Any thoughts?

@rylnd
Copy link
Collaborator

rylnd commented Apr 13, 2018

@Jackymancs4 here are my thoughts:

  • The only real "resources" we have are an avatar, identified by /:id, a custom avatar, identified by /face, and some meta stuff that doesn't really belong elsewhere
  • I think everything else is a modification of the above, e.g. size or face components
  • I think we should condense this down to three endpoints with optional query parameters:
    • /:id, with optional size parameter
    • /custom, with optional size and default components that can be overridden by specifying a query param (e.g. /custom?eyes=eyes1)
    • /meta for list and random, with an optional size parameter

The change to include the resultant size in /list seems fine to me.

@brandonmack111
Copy link

brandonmack111 commented Apr 19, 2018

I notice that on your homepage you say "we'd love to feature more artists' series."
To me, this implies there will be room in the future for expansion. This means you should include some space in your schema to allow for this.

I feel that the :scope should be included as a way of future-proofing, so you can add more sets later (for example, say I created a set that I called 'monkey faces.' This could easily be added, then, by using the scope monkey, making the full URL for an identifier-based monkey face something like /avatars/monkey/[email protected] with an optional :size parameter, or other params in place of the id, of course.

@rylnd
Copy link
Collaborator

rylnd commented Apr 20, 2018

@brandonmack111 I agree!

So perhaps we need four top-level resources, then:

  • /custom as above
  • /meta as above
  • /series/:seriesName/:id for artists' sets
  • /:id as the backwards-compatible catchall

@brandonmack111
Copy link

You might also want to allow custom avatars in other sets, perhaps with /series/:seriesName/:custom/<params> - though that does mean you need a modification to your /meta/list as well, since different sets will have different resources in them. That could perhaps also have an optional :series param (as in /meta/list/:series)

I'm considering working on this one myself, since I actually want to put together a set for you, and this would need to be sorted out first.

@rylnd
Copy link
Collaborator

rylnd commented Apr 20, 2018

@brandonmack111 I think that there's a compromise between what is possible and what we can support as maintainers. Generating custom (not generated from an :id) avatars is not currently the core use case of this middleware. Custom avatars for arbitrary avatars series could exponentially grow the complexity here if not handled correctly.

I see one valid use case for the custom avatars: it allows clients the ability to override the hashing mechanism implemented by the middleware. If we instead make that piece of the codebase more modular and extensible, I would prefer that route to some arbitrary number of hardcoded endpoints.

@rylnd
Copy link
Collaborator

rylnd commented Apr 20, 2018

@brandonmack111 something like this:

var Avatars = require('adorable-avatars');
var avatarsMiddleware = Avatars({ hashFunction: myHashingFunction });

var myApp = express();
myApp.use('/myAvatars', avatarsMiddleware)

@brandonmack111
Copy link

brandonmack111 commented Apr 21, 2018 via email

@qaisjp
Copy link

qaisjp commented Aug 10, 2019

Or just API versioning

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

4 participants