-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
RFC: helpful error message for deferred bindings [WIP] #2109
Conversation
When using our authentication package, many users try to inject the current user directly via `@inject()`. Because this injection is resolved before the sequence is executed, and the current-user value is set only inside the sequence, such attempt fails with an error that does not provide any guide on how to fix it. In this commit, I am adding a new kind of bound value: deferred. It is serving as a placeholder for bindings that will be set later via `@inject.setter`. When users try to access such binding too early, we can provide a helpful error message pointing them to `@inject.getter`. TODO: - unit tests - optional injection - fix failing tests - documentation - a feature flag to preserve backwards compatibility
Ah yes I ran into this problem and figured it out after banging my head on the table. |
@bajtos I don't think we have to introduce a new API. It can be achieved using export const toBeBound = binding => {
binding.toDynamicValue(() => Promise.reject(
new Error(
`There was no value provided for "${
binding.key
}" yet. Consider using \`@inject.getter()\`.`,
),
);
}
ctx.bind(AuthenticationBindings.CURRENT_USER).apply(toBeBound); Or maybe it can be even more simpler by defaulting ctx.bind(AuthenticationBindings.CURRENT_USER); // without further calls, it will be resolved to a rejected promise with meaning error information. |
@raymondfeng I prefer the code to be easy to write & understand. When I read The option
IMO: Bindings with a value set later are an integral part of Context, similarly to |
On the second thought, I am no longer convinced this is such a good idea. It turns out that To make this feature useful, I think we would have to offer a more complex advice, e.g. check that the sequence is calling authenticate action, etc. I am going to close this pull request for now, we can revisit it later if we learn about other scenarios when users encounter Error: The key XYZ was not bound to any value. |
When using our authentication package, many users try to inject the current user directly via
@inject()
. Because this injection is resolved before the sequence is executed, and the current-user value is set only inside the sequence, such attempt fails with an error that does not provide any guide on how to fix it.In this pull request, I am introducing a new kind of bound value: deferred. It is serving as a placeholder for bindings that will be set later via
@inject.setter
. When users try to access such binding too early, we can provide a helpful error message pointing them to@inject.getter
.Such improvement is relying on the good will of extension authors to create a deferred binding for a value they will set later using
@inject.setter()
. I think that's too much to ask and we should provide a way to enforce this behavior.The second part if this pull request is modification of
@inject.setter
to report a warning when the binding key is now know (was not registered as deferred). Originally, I want tothrow new Error
, but that would break backwards compatibility (which I don't want to right now).TODO
@strongloop/loopback-next what do you think? If you have encountered Error: The key authentication.currentUser was not bound to any value., would you find it useful if the error message told you to use
@inject.getter()
?Related issues:
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated