Make service containers readonly #1635
Conversation
| const obj: any = Object.seal(inject({})); | ||
| expect(Object.isExtensible(obj)).toBe(false); | ||
| expect(() => (obj.a = 1)).toThrowError('Cannot define property a, object is not extensible'); | ||
| expect(() => (obj.a = 1)).toThrowError('Cannot set property on injected service container'); |
There was a problem hiding this comment.
Looks a bit weird because a was not initially set to a value.
I would assume that any property that is not defined on this object is inaccessiable.
There was a problem hiding this comment.
Without the set override on the proxy it was previously completely valid to set the value. This test ensures that it's not possible.
| getOwnPropertyDescriptor: (obj, prop) => (_resolve(obj, prop, module, injector || proxy), Object.getOwnPropertyDescriptor(obj, prop)), // used by for..in | ||
| has: (_, prop) => prop in module, // used by ..in.. | ||
| ownKeys: () => [...Reflect.ownKeys(module), isProxy] // used by for..in | ||
| ownKeys: () => [...Object.getOwnPropertyNames(module)] // used by for..in |
There was a problem hiding this comment.
What is the difference here? The documentation says it is the same, except that symbols are not listed with Object.getOwnPropertyNames.
What is the idea behind this change? On first sight it looks more complicated, because you need to handle isProxy more.
There was a problem hiding this comment.
isProxy is a very special variable that should not be exposed from the service container using reflection. We only need it internally.
| @@ -28,7 +28,7 @@ import { DefaultWorkspaceSymbolProvider } from './workspace-symbol-provider.js'; | |||
| * Context required for creating the default language-specific dependency injection module. | |||
| */ | |||
| export interface DefaultModuleContext extends DefaultCoreModuleContext { | |||
| shared: LangiumSharedServices; | |||
| readonly shared: LangiumSharedServices; | |||
There was a problem hiding this comment.
Since you are throwing an Error on the setter of the proxy, would it be a good idea to require that all properties of the Module are readonly?
There was a problem hiding this comment.
I'm not sure it's possible to enforce this on the type level in TypeScript.
|
I just saw that my code is no longer working and saw that this change caused it. I used to create my own |
|
@pbrostean You're supposed to use the |
|
@msujew I've alreadly tried using the |
|
simply does not work? |
|
It does, thank you! I don't know why I had it in the declaration of custom services. Is there an equal implementation for a custom |
|
@pbrostean Yes, but the command handler needs to go into the shared services: export const YourDslSharedModule = {
lsp: {
CommandHandler: services => new YourDslCommandHandler(services)
}
} |
|
@msujew @cdietrich you two are the best, thank's for the super fast replies! |
Fixes something that was encountered in #1621 (reply in thread).
Essentially, our API (and runtime as well - to some degree) allows to override the values of services simply by performing assignments. However, this almost certainly breaks runtime behavior in case the service that is being assigned is used as a service dependency somewhere.
This change adds readonly modifiers where necessary and adds a
setmethod to the service proxies that ensure that no service can be assigned.