CrowdHailer / raxx

Interface for HTTP webservers, frameworks and clients
https://hexdocs.pm/raxx
Apache License 2.0
400 stars 29 forks source link

New 1.0 roadmap #167

Closed CrowdHailer closed 5 years ago

CrowdHailer commented 5 years ago

This is the follow up to #118, it was decided at least one more breaking release should be done before 1.0

Checklist

Optional Checklist

CrowdHailer commented 5 years ago

Thanks to @lasseebert and his work to remove EExHTML as a dependency (#171) we can drop a 1.0 for EExHTML as a requirement for Raxx 1.0

CrowdHailer commented 5 years ago

Nearly ready for a 1.0 release :tada:

Try the release candidate from hex using, {:raxx, "~> 1.0.0-rc.3", override: true}

My opinions on all that remains is.

  1. Don't do anything relating to being able to choose the reason phrase in a response. It can be added as a non breaking change after 1.0
  2. remove the final skipped test the router should not make assumptions about what metadata to add as it is intended as a base layer on top of which more sophisticated routing can be built
  3. Leave raw_path as it is now.
nietaki commented 5 years ago

I took a quick look through the project and recent changes and There don't seem to be any real blockers for 1.0.

Random notes:

  1. I don't think any changes to Middleware/Stack should be breaking for any users. The only thing that can change is the Stack state structure, which is appropriately marked and shouldn't really be relied upon by anyone.
  2. I didn't look at all of the recent code changes, but the changelog looks reasonable.
  3. Removing the skipped test makes sense. More involved routers with parameter bindings and conventions can be developed later on, with no changes to Raxx core.
  4. I'm all for leaving raw_path as it is now.
  5. I can try the RC with some old dusty projects this weekend, see if there's any warnings or anything when using it.

One thing that I think might be worth addressing before 1.0: Configuring extra statuses using config/Application.get_env - I don't think there's much value to it, it's the only instance where Raxx depends on the config and I can imagine situations where it leads to some stupid problems. I think I'd be in favour of removing the extra_statuses altogether before 1.0 to not carry it on as baggage.

CrowdHailer commented 5 years ago

Thanks for the comments.

I don't think we can remove extra_statuses without implementing the replacement. We use it and it hasn't caused any problems for us. but that's all I have. I don't think it's the best solution I'm just also not sure it's worth stopping 1.0 for it??

nietaki commented 5 years ago

What I meant with extra statuses is that we could allow for arbitrary {status_code_integer(), status_name_string} tuple in the %Raxx.Response{} status field (and any functions that take the status as an atom or integer).

This way the user could still send arbitrary statuses, but without the need for a global config. It wouldn't be difficult to use either, in your user code you could just define def unprocessable_entity(), do: {422, "Unprocessable Entity"} and use it wherever needed.

I think this would be a clean solution, requiring minimal code changes and making Raxx independent of Mix.Config. Plus I think that the assumption that everything in the user app would like to share the extra statuses might not always be correct.

CrowdHailer commented 5 years ago

I like the proposal. But does it need to be a thing before 1.0? Raxx.request() currently takes atom | integer after it would take atom | integer | {integer, binary That could be added at 1.1?

nietaki commented 5 years ago

My reply from 2 days ago didn't send :/

There's a couple of reasons why I'd like to get that in before 1.0:

CrowdHailer commented 5 years ago

Alright I'm sold. I do think that when this gets implemented it should go as a 0.19.x release. And exists for a bit in the wild. I will at least get feedback from using it in my own projects

CrowdHailer commented 5 years ago

It is worth noting that HTTP/2 does have reason phrases and HTTP/1.1 can have responses with no reason phrase and you can already pass any integer to Raxx.response

So any status code is already supported by just passing an integer when making the response. I wonder what plug/phoenix/cowboy does

CrowdHailer commented 5 years ago

Going on the points above I have added this test. https://github.com/CrowdHailer/raxx/pull/172/commits/f01963748f52796b063c441a3de9f3b97d9b6a5c

In my opinion that shows that we can handle any status code. if you want words then define a function that returns an integer and use as follows

def my_status(), do: 299

Raxx.response(my_status()

I don't think there is any need to support custom status wording.

CrowdHailer commented 5 years ago

Here's the commit on the next branch https://github.com/CrowdHailer/raxx/pull/172/commits/dd2ee03e54e785d5dac31bce98e7a0b7e45bd8a6