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

RFC: breaking change to improve the default behavior of get / #973

Closed
ryanblock opened this issue Oct 2, 2020 · 8 comments
Closed

RFC: breaking change to improve the default behavior of get / #973

ryanblock opened this issue Oct 2, 2020 · 8 comments
Assignees
Labels
@deploy @architect/deploy enhancement @package @architect/package @sandbox @architect/sandbox
Milestone

Comments

@ryanblock
Copy link
Member

ryanblock commented Oct 2, 2020

Currently, @http get / acts as a greedy catchall at the root – any requests not specifically caught by other routes get forwarded to your src/http/get-index handler. For example:

@http
get /
get /foo
  • get requests to / go to get /
  • get requests to /foo go to get /foo
  • post requests to /foo go to get /
  • get requests to /bar go to get /
  • etc..

Over time, we've heard a lot of feedback related to confusion as to why get / is special and magical in this way. The decision behind this behavior made partly because earlier in Architect's life:

  • We did not have support for greedy routes (where a single handler can greedily capture all requests underneath its current path)
  • We did not yet have support for wildcard (any) http methods (where a single handler is responsible for any / all http methods)
  • We frequently heard the feedback that we needed an easy way to build SPAs, and also that we ought to some way to backstop requests not caught by existing routes (presumably the same feedback that led AWS to add $default support in HTTP APIs).

Now the landscape has changed! As of this week's latest RC, we now have support for:

  • New catchall syntax, ex: get /* (which captures all get requests beneath root)
  • New any, options, and head methods, ex: any / (which captures all HTTP methods to /)

Combining the two – any /* – gets us the current get / greedy catchall behavior. This gives us the opportunity to make get / literal, and allow folks to opt into that behavior by defining any /*.

Doing so would be a breaking change, albeit a minor one. The upgrade path would be: if you use get / and you rely on get / to do more than just handle get requests to /, then you must:

  • Change @http get / to any /*
  • Rename your src/http/get-index folder to src/http/any-catchall

Impacts of this change

Customer impact

Minor; generally this should address very common feedback making get / behave as people would expect. Remediation for the breaking change would be to change a folder name.

Code impact

Sandbox

The Sandbox 2.0 refactor included a decent amount of work around HTTP APIs use of $default, which I'll need to refactor. I'd say the Sandbox work will be the meatiest portion of this change. Not sure how long it'll take, but Sandbox is now so extremely surrounded in tests I'm not sweating it.

Package & deploy

This is, on its surface, a quick change for Package and Deploy, and most of the work is already done. However, I think this change should be backwards compatible to ease folks migrating from Arc 6 directly to Arc 8, and that means adding support for catchall * syntax and any syntax to deploy, so that's going to add some work I wasn't expecting to do. Should be too bad, but it'll take a bit of time.

Everything else

Likely no (or only minor) impact.

Docs impact

Minor, mostly additive.

That's it. I'd appreciate feedback on this change (and a corresponding bump to Architect 8)!

@ryanblock ryanblock self-assigned this Oct 2, 2020
@brianleroux
Copy link
Member

brianleroux commented Oct 2, 2020

++ for Architect 8.x

@ryanblock
Copy link
Member Author

ryanblock commented Oct 2, 2020

Forgot to add the change impacts... adding now!

@kristoferjoseph
Copy link
Contributor

kristoferjoseph commented Oct 2, 2020

I am normally against any kind of breaking change, but this one makes a lot of sense.

@filmaj
Copy link
Member

filmaj commented Oct 2, 2020

It feels like a necessary and positive change for me, though I am also loathe to introduce breaking changes.

How do conflicting combinations of HTTP verb and path parameters interact or take precedence? e.g. any /* and post /api/user/:id, or any /api/* and post /api/?

@ryanblock
Copy link
Member Author

ryanblock commented Oct 2, 2020

@filmaj short answer: conflicting combinations are dealt with in AWS by API Gateway (examples below), and in Sandbox by order in Arc manifest (until I at some point get around to the gnarly work of emulating their route fallthrough logic, which is intense). Anyway, assuming:

@http
any /*
post /api/user/:id
  • post requests to /api/user/whatever hit post /api/user/:id
  • All other requests hit any /*

Assuming:

@http
any /api/*
post /api/
  • I believe all requests to /api/whatever hit any /api/*, and post /api/ is effectively ignored (I don't recall if catchall would also capture a direct post to exactly /api/ though).

@ryanblock
Copy link
Member Author

ryanblock commented Oct 2, 2020

@kristoferjoseph super agree. I've been thinking of ways to do this that wouldn't be breaking, and I just can't come up with anything, unfortunately.

@brianleroux
Copy link
Member

brianleroux commented Oct 2, 2020

we could be additive and create a new code path for opt in. a few ways to do that. new pragma (@https for example) or a new flag under @aws like catchall implicit|explicit but…I think this just adds more confusion and would create MORE work to migrate not only our examples but userland. (vs the relatively small amount of work to rename get-index to any-catchall.)

@ryanblock ryanblock added this to the 8.0.0 milestone Oct 8, 2020
@ryanblock ryanblock added @deploy @architect/deploy @package @architect/package @sandbox @architect/sandbox labels Oct 8, 2020
@ryanblock
Copy link
Member Author

ryanblock commented Oct 9, 2020

Mainlined and released tonight as 8.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@deploy @architect/deploy enhancement @package @architect/package @sandbox @architect/sandbox
Development

No branches or pull requests

4 participants