Adds error message for site with non-trailing asterisk #839
Conversation
EverlastingBugstopper
requested review from
ashleygwilliams,
ashleymichal,
exvuma and
gabbifish
src/commands/publish/mod.rs
Outdated
| @@ -37,6 +37,11 @@ pub fn publish( | |||
| validate_worker_name(&target.name)?; | |||
|
|
|||
| if let Some(site_config) = target.site.clone() { | |||
| if let Some(route) = &target.route { | |||
| if !route.ends_with("*") { | |||
| failure::bail!("The route in your wrangler.toml should have a trailing * to apply the Worker on every path.\nroute = {}*", route) | |||
There was a problem hiding this comment.
I'd use message::warn and have this message clarify that without the trailing *, your site may not work as expected.
There was a problem hiding this comment.
i agree, i don't think bailing is necessarily the right move.
There was a problem hiding this comment.
Yeah - if you see my original comment on the PR, I wasn't sure if we should bail or warn here. The reason I opted for bailing is because I'm 99% sure that it just breaks your site no matter what. If you do this the first time you're migrating your site from something else, your site will go down, even if you see the warning and fix it on the next push. I'm willing to change it to a warning, but I'm really not sure we want to go that "route" 😛
There was a problem hiding this comment.
#iseewhatyoudidthere
my kneejerk reaction is always to say "listen, we don't necessarily know what a user plans on doing with this". However in this case, you make a pretty strong point that a worker that does not use a wild card route here will likely never serve anything other than the root index.html, which would be kind of useless.
either way though, i think gabbi's warning of "might not work as expected" is important.
There was a problem hiding this comment.
even if they were only serving index.html and nothing else (which is highly highly unlikely), it would still be in their best interest to have /* - if they have other workers that are more specific elsewhere, they will take precedence. I really really don't think we're going to have users come complaining that they can't deploy a worker without the asterisk aside from the usual complaint that its confusing why its necessary at all, which we aren't solving for currently.
Edit: I think that failing here would save way more people from deploying a broken site than just a warning would, and I really really do not anticipate people being upset about this even if they are only serving index.html. If that is their use case, it'll still work with a trailing asterisk no problem.
There was a problem hiding this comment.
you've changed my mind. i do like the wording "might not work as expected".
There was a problem hiding this comment.
@ashleygwilliams I'd love to get your input on this
EverlastingBugstopper
force-pushed
the
avery/route-66
branch
2 times, most recently
from
d7ab082
to
4491cc5
Compare
EverlastingBugstopper
force-pushed
the
avery/route-66
branch
from
67f2dde
to
54fb83c
Compare
EverlastingBugstopper
force-pushed
the
avery/route-66
branch
from
9d22ddb
to
a4bcb4d
Compare
Fixes #814
This PR adds an error message for users deploying a site to a route without a trailing asterisk.
Open to suggestions on the error message itself, and if we think this should be a
message::warninstead of afailure::bail. The reason I have it bailing here is because I see absolutely no scenario where you would deploy a site without a trailing asterisk. Even for single page applications I believe you still need the worker to run on every route.