"Feature/Addition Host domain" #2185 was a breaking change. #2222
Comments
sio4
added
bug
|
Sure, what I said as a "breaking change" is: Previously, func init() {
gothic.Store = App().SessionStore
goth.UseProviders(
twitter.New(os.Getenv("TWITTER_KEY"), os.Getenv("TWITTER_SECRET"), fmt.Sprintf("%s%s", App().Host, "/auth/twitter/callback")),
facebook.New(os.Getenv("FACEBOOK_KEY"), os.Getenv("FACEBOOK_SECRET"), fmt.Sprintf("%s%s", App().Host, "/auth/facebook/callback")),
linkedin.New(os.Getenv("LINKEDIN_KEY"), os.Getenv("LINKEDIN_SECRET"), fmt.Sprintf("%s%s", App().Host, "/auth/linkedin/callback")),
github.New(os.Getenv("GITHUB_KEY"), os.Getenv("GITHUB_SECRET"), fmt.Sprintf("%s%s", App().Host, "/auth/github/callback")),
)
}For now, I think there are basically two directions.
If there are more people using the multi-homing feature than using If we choose the second option, I think we need to keep this in mind and it could be better to choose the first option later when we design and implement v1. |
|
Thanks for explaining it @sio4. I think we should take the first route. The purpose of that feature is to limit the app to a single domain (only for it), the first set of names that come to my mind are |
|
cc @dmuriel |
|
Yeah, then I will take a look at the related things and will file a PR. For the naming part, I think we can postpone the decision. For now, I think if the return of the function is the type of Currently, my ideal idea is that if the function could return a different type of object which can be used as App {
Options
Router
}
Router {
router *mux.Router
routes RouteList
}
func (a *App) Router(h string) * Router {}instead App {
Options
router *mux.Router
routes RouteList
}
func (a *App) Host(h string) *App {}During some initial investigation, I found that This issue was caused by Also, it seems like multi-homing is now working if I do Luckily, the document page is not updated yet (gobuffalo/docs#605) so maybe not many users using it :-) |
|
I wonder if we could use the Options pattern here. And allow groups and top level applications to be specified a host. The API would be something like: type ApplicationOption func(*App)
func (a *App) WithOptions(...Options)
// WithHost does both, setting the app.Host and the
// changes to the mux instance inside the App.
func WithHost(h string) AppOption {
return func(app *App) {
app.Host = h
// changes to the inner router.
}
}
// Then the usage would be
app.WithOptions(buffalo.WithHost("my.host.com"))I would love to see Buffalo evolve to use the options pattern instead of maintaining the Options struct. However changing application initialization is out of the scope of this conversation. |
PreparationBelow is the test application setup with two Virtual Hosts. The main points are:
adm := app.VirtualHost("admin.example.com:3000")
adm.Middleware.Clear()
adm.Use(paramlogger.ParameterLogger)
adm.GET("/", func(c buffalo.Context) error {
return c.Render(http.StatusOK, r.String("Welcome to Admin!"))
})
app.GET("/", HomeHandler)
app.GET("/home", HomeHandler)
ur := userResource{}
k := app.Resource("/kings", &ur)
o := k.Resource("/kongs", &ur)
o.Middleware.Skip(paramlogger.ParameterLogger, ur.Create, ur.New)
o.Middleware.Skip(popmw.Transaction(models.DB), ur.New)
g := app.Group("/group")
g.GET("/", func(c buffalo.Context) error {
return c.Render(http.StatusOK, r.String("Hey Group!"))
})
g.GET("/in", func(c buffalo.Context) error {
return c.Render(http.StatusOK, r.String("Hey Group In!"))
})
gg := g.Group("/deep")
gg.GET("/", func(c buffalo.Context) error {
return c.Render(http.StatusOK, r.String("Hey Deep under the Group!"))
})
api := app.VirtualHost("api.example.com:3000")
api.GET("/", func(c buffalo.Context) error {
return c.Render(http.StatusOK, r.String("Welcome to API!"))
})
api.GET("/home", func(c buffalo.Context) error {
return c.Render(http.StatusOK, r.String("Welcome to API Home!"))
})
api.GET("/ping", func(c buffalo.Context) error {
return c.Render(http.StatusOK, r.String("Welcome to API Ping!"))
})The result of the current versionbuffalo routes
buffalo task middleware
|
After the fix
Routing decisions:
|
Fixed multi-homing feature and related bugs. (#2222)
Description
Oops! #2185 was a breaking change.
Previously, also currently,
AppembedsOptionsandOptionshasHost. So till v1.18.1, we can access configured Host byapp.Hostand users could use it e.g. https://gobuffalo.io/en/docs/goth/.By this change, apps referenced the value by
app.Hostwill fail to get the hostname. (Actually, my project used that.)The workaround is to change applications by replacing
app.Hostwithapp.Options.Hostbut I don't think this is a good thing since we also allow users to access the other values with the same convention likeapp.Name.I will check how the change works and what could be a better fix. (considering both of changing the new function's name or add a new function to get the value (including multi-homed Host) or just let it as is.)
It seems like this is more likely a design issue by the way. Please provide any ideas or opinions.
The text was updated successfully, but these errors were encountered: