Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Started migrating routing configuration to the new configuration API #2186

Merged
merged 4 commits into from Jan 17, 2016

Conversation

thecodejunkie
Copy link
Member

Moved routing configurations over to the new configuration as part of #2000

  1. Added RouteConfiguration, RouteConfigurationExtensions and DefaultRouteConfigurationProvider
  2. Renamed EnableHeadRouting to ExplicitHeadRouting when it was moved
  3. Removed routing related settings from StaticConfiguration
  4. Removed HEAD routing configuration check in NancyModule.Head (see discussion below)
  5. Added the INancyEnvironment onto NancyContext ⚠️

Old description

Currently what's left is to figure out the best approach to get the INancyEnvironment into NancyModule because it is currently used by the Head declaration to throw an exception if you try to declare HEAD route while it's disabled

I think our options are one of the following

  1. Add it to INancyModule (feels icky)
  2. Add it to NancyContext (feels less icky, and makes it accessible wherever the context is)
  3. Remove the check & exception from NancyModule

I think option 3 is the most compelling because the DefaultRouteResolve also makes use of the setting to determine if it should translate HEAD requests into GET requests. Removing the check & exception would only mean that you'd not get an exception (which is a bit weird anyway) if you try to declare HEAD requests while it's disabled.. it will just mean they'll never been able to be called until you toggle the setting

@jchannon
Copy link
Member

what would you get for option 3? 405 response?

@thecodejunkie
Copy link
Member Author

@phillip-haydon
Copy link
Member

IMO this check should be done before it ever gets to the Module...

I vote 3.

@grumpydev
Copy link
Member

I'd prefer option 2, feels like something that would benefit from being on the context anyway.

@thecodejunkie
Copy link
Member Author

@grumpydev I kind of thing 2 & 3 would be the way. I think throwing the exception is stupid. It means you can't temporarily toggle that feature on/off without making modification (even if that only means commenting out code) or it will throw exception at startup. I would vote 3 for this PR and then we create a new issue for 2 ?

@grumpydev
Copy link
Member

Only problem with removing the exception is that people could then add HEAD routes and they just wouldn't work with no indication as to why. I agree the exception should go, but I guess only when we fix the routing so it will give custom HEAD routes precedence over the default stuff, although I'm not sure why we don't already do that as that seems the "correct" thing to do.

@thecodejunkie
Copy link
Member Author

@grumpydev I'm trying to limit the scope so we can ship 2.0, not expand it.. unless you're offering to get a PR in for that behaviour? :D

@grumpydev
Copy link
Member

I'm just saying don't remove the exception yet until we've fixed it properly, so stick it in the context and log an issue to remove the exception/improve routing later :)

@thecodejunkie
Copy link
Member Author

@grumpydev you're just trying to avoid doing any work! ;)

@grumpydev
Copy link
Member

Avoid, no, postpone, yes :p

@phillip-haydon
Copy link
Member

Can someone explain to me why it belongs in the module or context when this AFAIK dictates if routing should occur?

@grumpydev
Copy link
Member

We are talking about the nancy environment object (and hence config in general) not this particular setting @phillip-haydon

@grumpydev
Copy link
Member

The setting itself should go away, but not yet.

@phillip-haydon
Copy link
Member

In that case option 2 and remove that setting when we can because it makes no sense

@thecodejunkie thecodejunkie added this to the 2.0 milestone Jan 17, 2016
@thecodejunkie thecodejunkie changed the title [WIP] Started migrating routing configuration to the new configuration API Started migrating routing configuration to the new configuration API Jan 17, 2016
@thecodejunkie
Copy link
Member Author

No longer WIP. Updated description. Ready to be reviewed and pulled ping @NancyFx/most-valued-minions @NancyFx/nancy-core-contributors

@phillip-haydon
Copy link
Member

👍

Looks much cleaner than the old statics everywhere.

@thecodejunkie
Copy link
Member Author

Added one more thing to the description (Step 5)

@jchannon
Copy link
Member

:shipit:

jchannon added a commit that referenced this pull request Jan 17, 2016
…ting

Started migrating routing configuration to the new configuration API
@jchannon jchannon merged commit 8970ac9 into NancyFx:master Jan 17, 2016
@thecodejunkie thecodejunkie deleted the configuration-migrate-routing branch February 17, 2016 07:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants