Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Alewis/refactor routes #918

Merged
merged 6 commits into from
Dec 2, 2019
Merged

Conversation

ashleymichal
Copy link
Contributor

@ashleymichal ashleymichal commented Nov 27, 2019

More intermediate refactors as we move toward multiroute support.

  • Move Route struct into settings/target
  • Make the routes key in Target a Vec rather than a HashMap
  • add a 'routes' method to Target, which will handle both route setting and routes setting, asserting that only one can be specified in the wrangler.toml, and generalizing them to a Vec

TODO:

  • memoize/cache calls to get all routes; I would like to only refresh these on 10020 errors when we try to create a route, rather than every time we try to upload a specific route
  • add tests (this PR)

@ashleymichal ashleymichal added refactor regression Something is broken, but works in previous releases status - needs review labels Nov 27, 2019
@ashleymichal ashleymichal added this to the Multi Route Support milestone Nov 27, 2019
@@ -31,7 +31,7 @@ pub struct Manifest {
pub workers_dev: Option<bool>,
#[serde(default = "some_string")]
pub route: Option<String>,
pub routes: Option<HashMap<String, String>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interested in why this was a hashmap to begin with - i know we never used it anywhere but still interested

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because technically routes are a map between a pattern and a script, and it is potentially useful within a Wrangler project if you wanted to "negate" a route by setting it to a null worker. For the purposes of this refactor, I am starting with the simplest implementation - a list of patterns - to reach parity with serverless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense - will this bite us at all when we try to migrate? will the future require breaking changes to the toml? would like to avoid that as much as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

predicting the future of routes is about as easy as predicting next year's weather (probably will be hotter than today, but who knows if it'll be raining). That being said, if it is clear before release that we need this data structure to change we can do that.

let mut routes = Vec::new();

if let Some(single_route) = &self.route {
if self.routes.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this case isn't covered in the else and should probably be brought out to the top -

if &self.route.is_some() && &self.routes.is_some() {
  failure::bail!("You can specify EITHER `route` or `routes` in your wrangler.toml")
}

(also it shouldn't have a trailing semi-colon i think)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean "this case isn't covered in the else"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the else if let Some(multi_route) block, we do not check to see if there is also a route field. I see now that if they were both there, the first block would catch it, but I still find it a bit strange that we check if both exist after we check if one exists. I think I'd have it as its own separate block like I describe above even though they would be functionally equivalent.

Copy link
Contributor Author

@ashleymichal ashleymichal Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah covering it in both places would be redundant. i'll pull up the first case. is that really blocking then?

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay :):)L):):): I:P):)):)

@ashleymichal
Copy link
Contributor Author

sob. all my tests are borked. cloudflare-rs should be published sometime today i think

@ashleymichal ashleymichal merged commit 1b99230 into feat-multi-route Dec 2, 2019
@delete-merged-branch delete-merged-branch bot deleted the alewis/refactor-routes branch December 2, 2019 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactor regression Something is broken, but works in previous releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants