tc39 / proposal-decorator-metadata

MIT License
140 stars 9 forks source link

Updates proposal to the latest design #12

Closed pzuraq closed 1 year ago

pzuraq commented 1 year ago

Updates the proposal to pass a simple shared object between all decorators, which gets assigned to Symbol.metadata in the end of class definition.

zloirock commented 1 year ago

So Symbol.metadata or Symbol.metadataKey? Both are still present here.

bakkot commented 1 year ago

I really think this should be frozen. As I've mentioned before, I really don't think it's a good idea to introduce a new shared string-based global namespace, which is what a mutable metadata object would amount to.

Feel free to merge this, of course, but I want to register my continued opposition.

pzuraq commented 1 year ago

@bakkot several members have now stated that a frozen object will not solve their use cases, and that the proposal would not be able to advance in that form, specifically because they need the ability to share metadata publicly without using a WeakMap. TypeScript is the main proponent of this, but there have been a few others. We've debated this in the decorators meeting a few times, and the conclusion has been that a frozen object will not be a powerful enough API.

That said, I will be presenting the frozen object as a potential alternative at the upcoming plenary, because you and a few others have expressed strongly in the opposite direction. In addition, there is a third option, which is to use getter/setter functions to enforce usage of Symbol keys instead of allowing any key, which would discourage accidental namespace collisions but still enable intentional sharing of metadata.

See the slides for the upcoming presentation for more details there.

bakkot commented 1 year ago

@pzuraq Hm, when we left off the discussion, I was under the impression that TypeScript's use case could be adequately (if slightly awkwardly) met by a frozen object. And it must be said that their use case (with the constraint that the decorator is injected by a compiler, rather than provided by a library) is extremely unusual; if it's only their particular case which requires it, I would not consider that adequate grounds to adopt what seems to me to be a much inferior design.

So I hope that there's more details about use cases which are not met by frozen objects included in the presentation. The slides don't currently cover that. There's just "No way to share public metadata, e.g. type information, to be used by multiple parties", which is false as stated; a decorator library can export an API which provides access to that metadata for any consumer, as we've discussed.

pzuraq commented 1 year ago

@bakkot it turns out that sharing metadata intentionally and publicly is a much more common use case than previously realized. This is especially true for type-based metadata, but it's also true for other information such as validation, serialization/marshaling, ORMs, etc. where it is useful to have information about the types of values and what is valid/invalid.

The desire from authors is specifically that they do not have to expose a library to access this information. There are a few reasons for this:

  1. A frozen object approach would just move the namespace collision/coordination problem from the metadata object itself to NPM. Multiple libraries would likely be published and used for various types of information, e.g. types. If a decorator did not support a specific library, then they would be incompatible. This would either result in diverging ecosystems, or decorators having to support many different libraries for metadata.
  2. Multiple versions of the same library being included in a build can easily result in metadata that is meant to be shared not being shared/becoming sharded. This requires extra design to make sure it does not happen, e.g. by adding the WeakMap containing the metadata to the globalThis namespace, and extra overhead in making sure that that global API does not change.
  3. Type information is especially common to use, both via native JavaScript and via emit from a separate build tool like TypeScript. The frozen object design would force decorators to support two different ways to get the type information, via the patched context object in your example and via a shared library. Putting aside the ecosystem coordination issue of using a shared library, requiring every single decorator to support both patterns is likely to cause a split in the ecosystem. It would likely result in many decorators that only support TypeScript's (or any other build tool's) version of a patched context object, or that only support using a specific shared type library.
bakkot commented 1 year ago

Thanks for elaborating. None of that is convincing to me, though - in the first two of your bullet points, identical arguments apply to just sticking all one's exports as string keys on the global object. And it's true that doing so does have some benefits. But those benefits do not come close to outweighing the costs. That is to say, the appropriate way to handle the coordination problem is npm, which allows names to be actually globally unique (i.e., across all programs, not merely within a program), and which supports renaming and so on to handle versioning and collisions. "You can simultaneously use multiple versions of a library without them colliding" is a feature, not a bug.

And the third bullet point is unique to TypeScript, and their unwillingness to emit code which depends on a library. But there's plenty of other ways to solve their particular constraint - since they're already changing the runtime semantics of code, they could for example just stick a helper on the global object. The only downside of sticking stuff on the global is that it's a shared namespace, but it seems to me that it would be better if it were only TypeScript which would have to deal with navigating a shared namespace, rather than making everyone have to deal with that.

pzuraq commented 1 year ago

@bakkot

But those benefits do not come close to outweighing the costs

Can you help here by elaborating on what the perceived costs would be here? There are very real concerns and outlined costs against a frozen object, but there has not been much discussion of what the costs of a mutable object would truly be.

Perhaps I and others have not had enough experience with dealing with global namespaces, but this axiomatic approach so far feels like its not genuinely trying to solve the real problems that this proposal is addressing, and is instead following an unspoken rule of sorts. It also feels like no burden of proof would be enough to convince you here, which is why we ultimately decided to go to plenary with the simpler version of the proposal rather than a compromise, as that does not appear to be possible.

Can you outline specific issues you would like to avoid that you see occurring? Preferably with examples that have occurred in other global namespaces?

bakkot commented 1 year ago

Can you help here by elaborating on what the perceived costs would be here?

Sure: the main cost is that now any library containing a decorator which provides metadata needs to coordinate with every other such library to avoid name collisions, including all other versions of one's own library. That is, of course, impossible, so in practice this boils down to "completely unrelated bits of code which each work on their own mysteriously do not work when used in combination". This is bad for everyone. It's exactly the same reason we use module exports to manage the rest of our APIs, instead of blindly sticking stuff on the global object. There's no meaningful difference between that case and decorator metadata that I can see.

(Also, it makes metadata public by default, which is usually a bad idea. This is bad, but not as bad as the shared namespace.)

I hope at this point that "no new shared string-based global namespaces" isn't an unspoken rule; I've been pretty explicit about it. (This also isn't the first time it's come up; that was one of the major things which prevented advancing the mixins proposal, for example.) Certainly it is possible that such a thing might be the best possible design, in which case we'd have to violate this rule (or give up on whatever feature required that design), and I am open to further arguments that this is the case here. I'm just not convinced of that right now - none of the discussion so far has made it clear to me what is special about metadata that renders inadequate the existing import/export mechanism we generally use for coordinating, except in the unique case of TypeScript (which, as I've said, can meet their unique constraints in other ways).

pzuraq commented 1 year ago

the main cost is that now any library containing a decorator which provides metadata needs to coordinate with every other such library to avoid name collisions, including all other versions of one's own library. That is, of course, impossible, so in practice this boils down to "completely unrelated bits of code which each work on their own mysteriously do not work when used in combination".

This is demonstrably not true. With the mutable object approach, it is trivial for authors to avoid unintentional collisions, either weakly via an unexported Symbol or strongly via a WeakMap. It is not impossible.

It's exactly the same reason we use module exports to manage the rest of our APIs, instead of blindly sticking stuff on the global object. There's no meaningful difference between that case and decorator metadata that I can see.

There is a meaningful difference.

The purpose of public metadata is coordination in a broad ecosystem of different libraries. The point is that these libraries want to share a minimal amount of information with each other so that they can produce the desired effect - this is a requirement.

The purpose of module exports is to contain a piece of code which should only be used in one place - by whatever consumer imports it. There is no question of ecosystem coordination, there is no need to synchronize library versions or all decide to use the exact same library for a given use case. Multiple libraries can be included that do the same thing without impacting the overall system at all. This is why build tools can either dedupe or tree shake modules (or not) depending on various heuristics. This would never be possible with metadata.

Attempting to force the use case of metadata into the use case of modules will result in a far more complex ecosystem problem, with many more instances of "completely unrelated bits of code which each work on their own mysteriously do not work when used in combination".

Also, it makes metadata public by default, which is usually a bad idea. This is bad, but not as bad as the shared namespace

I would argue that requiring author's to coordinate via modules forces them to rely on the implementation details of build tools and the ways modules are packaged, deduped, etc. These processes are not spec'd by the language and can vary significantly, and would lead to a far larger version of the issues noted by Hyrum's law, in my experience.

Or, alternatively, it will lead to libraries placing all metadata on the global object to avoid these issues, which is even worse in terms of namespace collisions.

I hope at this point that "no new shared string-based global namespaces" isn't an unspoken rule; I've been pretty explicit about it.

I don't think I was clear enough. The unspoken part is the criteria by which a new shared string-based global namespace could be added.

As you yourself note, it is possible that it could be the best design. We have presented multiple reasons why we believe it is the best possible design, and the feedback has been very vague. Can you outline what the requirements would be to introduce such a namespace?

In addition, you have not addressed the other proposed alternative - using a symbol-based namespace instead of a string-based one. This would address the "public by default" issue by making it semi-public by default instead, while still retaining the other benefits. Would this solution be an acceptable compromise, as it addresses most of your concerns re: a global namespace?

zloirock commented 1 year ago

What about adding some methods for work with metadata like it was in Reflect metadata? Even in your presentation, you use some similar helpers. Most who will use metadata will be forced to implement it by themself.

pzuraq commented 1 year ago

@zloirock that is the Symbol based alternative I'm discussing above, essentially.

rbuckton commented 1 year ago

I really think this should be frozen. As I've mentioned before, I really don't think it's a good idea to introduce a new shared string-based global namespace, which is what a mutable metadata object would amount to.

Feel free to merge this, of course, but I want to register my continued opposition.

I've gone to great lengths in https://github.com/tc39/proposal-decorator-metadata/issues/5 to explain why a frozen metadata object is a non-starter for TypeScript's use cases, which is one of the largest use cases of metadata (--emitDecoratorMetadata and the reflect-metadata library) that currently exists in the JS/TS ecosystem. Having a mutable object does not preclude any of the capabilities you want to leverage (i.e., using a WeakMap with the object as key), but a frozen object would be a significant impediment to TypeScript's use cases and would be a major blocker for migrating what are potentially thousands of existing projects to native decorators and native metadata. We are strongly opposed to a frozen object.

rbuckton commented 1 year ago

I really think this should be frozen. As I've mentioned before, I really don't think it's a good idea to introduce a new shared string-based global namespace, which is what a mutable metadata object would amount to.

To further clarify, we are perfectly willing to allow the metadata object to be frozen after decoration, just not during decoration.

zloirock commented 1 year ago

@pzuraq I'm not sure that it depends on it - such built-in helpers make sense with any storage design.

rbuckton commented 1 year ago

@pzuraq Hm, when we left off the discussion, I was under the impression that TypeScript's use case could be adequately (if slightly awkwardly) met by a frozen object.

I apologize for not having responded further in that thread, but no, we do not feel that solution is adequate. Patching the context object is a no-go, as we're trying to avoid introducing new TypeScript-specific runtime emit. That is something that has been strongly contraindicated by many in the JS/TS community as well as other members of TC39. Such patching would also result in decorators that only work when used in TypeScript, resulting in further bifurcation of the ecosystem. It does not align with our goals, nor with the goals of proposals like Type Annotations.

Yes, having a mutable metadata object has the potential to allow for collisions, but that can be addressed by using a WeakMap. Not all cases necessitate this, however, so enforcing a frozen object only increases the barrier to entry for decorator authors.

In addition, enforcing uniqueness with keys is fraught with issues. The @appendMeta example in my review would not be possible if you were forced only to write unique keys each time.

We don't need to enforce a frozen object/WeakMap constraint, or even a uniqueness constraint, on the entire ecosystem. Those that need to ensure uniqueness can use Symbol. Those that need to ensure isolation can use WeakMap. Those that need to facilitate data sharing, or those that need to run in a Script, or those that can't mutate globalThis (such as when used in SES, post lockdown()), can use string-based keys (or Symbol.for, which is really no different from a string-based key). An artificial limitation such as you are suggesting only limits perfectly valid uses cases.

pzuraq commented 1 year ago

@zloirock to elaborate a bit:

  1. Using the Reflect.metadata API doesn't really solve the problem, because it would still be a global string-based namespace, it would just be in a different place right now.
  2. We could limit the API to have certain requirements, e.g. keys must be Symbols, to prevent collisions by default. This is what I meant by it being effectively the same as the Symbol based alternative.
  3. In addition, introducing a new global API increases the size of the problem significantly. We would need to add new APIs for intercepting metadata (e.g. Proxy handler methods, potentially) whereas assigning metadata to a standard property allows us to to rely on all the existing behaviors for accessing a property.
zloirock commented 1 year ago

@pzuraq I didn't say that it should be Reflect.metadata API - it should be just an API for common cases like helpers in your examples. Sure, if it's required - with limitations like symbols. An exhaustive API is not required - I just say about some basic helpers. An API for proxy handlers is not required at all in the case of simple semantics like it's explained in this PR. With simple semantics more complex / specific helpers can be implemented in the userland.

pzuraq commented 1 year ago

@zloirock all new global APIs boil down to the same set of problems which, yes, does include Proxy handlers. This has been called out by other TC39 members - we originally proposed a while back adding window.Metadata and that was the blocked for that reason.

zloirock commented 1 year ago

I can understand the requirement of proxy handlers / traps in case of complex semantics, but in the case of just well-known symbol and [[Get]] I don't see any sense of this since here internally used already existing internal objects methods. Helpers are required only for simplification operations on this.

pzuraq commented 1 year ago

@zloirock oh, I think I misunderstood your original point, you want to keep the underlying mechanism the same (e.g. metadata object gets assigned to Symbol.metadata on the class) but just add some helper methods that manipulate the metadata more easily.

I can't really see how that would solve the problems in this thread. If we were to add helpers that make a global WeakMap based on metadata keys that anyone could access, then it would still have all the same collision issues that @bakkot is describing. If we didn't, then it would have the same issues with sharing and such that @rbuckton and others have called out. If I'm missing something there, maybe you could add some examples of the APIs you're considering and describe why they would be helpful?

rbuckton commented 1 year ago
  1. We could limit the API to have certain requirements, e.g. keys must be Symbols, to prevent collisions by default. This is what I meant by it being effectively the same as the Symbol based alternative.

Symbol-based keys seems like an unnnecessary extra step. Symbols do not guarantee uniqueness, given Symbol.for, and if Symbol.for were also to be disallowed then that wouldn't resolve TypeScript's concerns. Allowing Symbol.for does little to address any concerns about a "shared namespace", and instead just serves to punish shared metadata cases like those TypeScript wishes to continue to support, since what could have been MyClass[Symbol.metadata]["design:type"] now requires the added overhead of MyClass[Symbol.metadata][Symbol.for("design:type")], so I don't particularly see the value.

  1. In addition, introducing a new global API increases the size of the problem significantly. We would need to add new APIs for intercepting metadata (e.g. Proxy handler methods, potentially) whereas assigning metadata to a standard property allows us to to rely on all the existing behaviors for accessing a property.

The API exposed by reflect-metadata is at odds with projects like SES as it not only provides a "shared namespace", but one that can be mutated at any time regardless of when lockdown() occurs. In the case of something like obj[Symbol.metadata], the metadata object could conceivably be frozen to prevent future mutation. Also, as @pzuraq says, reflect-metadata today does not pass through Proxy since reflect-metadata uses a WeakMap behind the scenes.

zloirock commented 1 year ago

@pzuraq it's not about solving the problem of collision and other semantics problems, it's just about avoiding duplication of helpers like appendMeta -) Such helpers make sense with any semantics.


Sure, I think that it's possible try to solve it with such helpers, for example:

const metadata1 = new Metadata();
const metadata2 = new Metadata();

@metadata1.append('a', 'x')
class C { }

metadata1.get(C).a; // => 'x'
metadata2.get(C).a; // => undefined

Each Metadata instance just uses its own unique symbol (not a weakmap or any other new kind of key) - this is observable, prevents collisions and does not require specific proxy traps. Anyway, it's just something that came to my mind in a few seconds, so I don't pretend to anything - my initial goal is far from solving this issue.

pzuraq commented 1 year ago

@zloirock I see. That pattern has been considered separately and has been determined to not work for the various use cases that metadata solves, for a variety of reasons. I think it's also out of scope of this PR, which is the design that has been agreed on previously.

If you want to continue discussion of alternatives like that, I suggest opening a separate issue to discuss.

zloirock commented 1 year ago

Ok, let's discuss it after solving the main semantics issue.

pzuraq commented 1 year ago

I'm merging this PR for the time being so I can update the agenda for the plenary with the new explainer, but we can continue the conversation here and open a new PR if any changes come up between now and then.