-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: default enable allowDynamicBackends with better unsupported errors #995
Conversation
9e35abd
to
8f79e16
Compare
a69a2ca
to
5b0712e
Compare
Two further suggestions that might make sense still here:
|
I've added the suggestion here in renaming to |
I especially like being able to set the implicit backend for any fetch() call via Given the security and configuration benefits of using static backends, and the fact that you can't see on the manage UI whether dynamic backends are enabled for a service, I'm a bit conflicted over whether to enable them by default. Our team began using dynamic backends, but has since switched our JS code to exclusively use static backends — despite this, IIRC, dynamic backends were left enabled on the services. This is a good reminder for us to fix that, but, for customers in our situation, this probably represents a breaking change worth noting. I had an idea while reviewing this; what if:
This would still let you use dynamic backends without enabling them twice, and provide some safety/control of third party dependencies, with the ability to opt out for convenience. But obviously it's a larger breaking change. |
Thanks for the thoughtful points here, it's really appreciated in trying to consider what API is best for users. I think the benefit of dynamic backends is that they allow code to run without modification from other providers, which may be useful for new users. It is perfectly valid though for us to conclude here that But since the security is already handled on the service-level, flipping the default seemed like it didn't have to tradeoff between both. On the other hand, as you say, being able to set the default implicit backend may be enough in many cases too. I'm not going to rush landing this as I think it requires some careful though, but hopefully we can work towards the right user experience going forward. |
Putting some more thought here - the Go SDK doesn't have a separate runtime-level flag itself, and neither does the Rust SDK, therefor I would like to move ahead with landing this. |
This PR is now ready to merge, I will land and release on Monday unless there is any further feedback. |
This changes the default in the JS SDK to remove the secondary
allowDynamicBackends(true)
call which is currently required in order to use dynamic backends, even when they are enabled at the service level.Intead, with this PR, calling
new Backend(...)
will automatically try to create a dynamic backend, returning a better error message when not enabled:Then secondly, when calling an arbitrary
fetch()
request without providing an explicitbackend
option, it will also try and create a dynamic backend automatically instead returning this new error message:Then to provide the previous security property of requiring explicit backends, we have a new
import { enforceExplicitBackends } from 'fastly:backend'
which can be used when users want to lock down their backend usage.We cannot override the service-level configuration from the JS SDK, so the default of them not being enabled remains, but we can make the default experience better without dynamic backends and remove confusion as to why it has to be "enabled twice" currently.
In addition, this PR also adds a new
Backend.prototype.name
readonly getter and deprecatesbackend.toName()
to usebackend.name
instead.Resolves 987.