Skip to content
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

Simpler Metadata #451

Closed
wants to merge 1 commit into from
Closed

Simpler Metadata #451

wants to merge 1 commit into from

Conversation

pzuraq
Copy link
Collaborator

@pzuraq pzuraq commented Mar 26, 2022

As was noted in @syg's recent review, and in previous issues raised on this repo, the current metadata format is fairly complex. This complexity was meant to solve a variety of ubiquitous use cases in the ecosystem of metadata-based decorators, but it also has led to a lot of bikeshedding and concern about being too opinionated and complicated. This proposal outlines an alternative metadata format which would ultimately be just as powerful, but significantly simpler.

README.md Outdated
@@ -105,7 +104,7 @@ The context object also varies depending on the value being decorated. Breaking
- `isStatic`: Whether or not the value is a `static` class element. Only applies to class elements.
- `isPrivate`: Whether or not the value is a private class element. Only applies to class elements.
- `addInitializer`: Allows the user to add additional initialization logic. This is available for all decorators which operate per-class, as opposed to per-instance (in other words, decorators which do not have kind `"field"` - discussed in more detail below).
- `setMetadata`: Allows the user to define some metadata to be associated with this property. This metadata can then be accessed on the class via `Symbol.metadata`. See the section on Metadata below for more details.
- `addMetadata`: Allows the user to define some metadata to be associated with this property. This metadata can then be accessed on the class via `Symbol.metadata`. See the section on Metadata below for more details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're not really associated with the property tho unless they're stored under the property name somehow - if it's just a bare array, then it's only associated with the class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, updated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was also an implicit question here :-) why not have the metadata value be an object, whose keys are property names, and whose values are arrays? To anticipate the “what about the class, or collisions between static and instance” part, what about an object like this:

{
  prototype: {
    foo: [],
  },
  static: {
    bar: [],
  },
  instance: {
    baz: [],
  },
  constructor: [],
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't work for private metadata, which was the whole reason we had the divide between public and private. This gets into the territory of "it's too complex" which seems to be a sticking point here, so this proposal is to be as simple as possible. Users can of course addMetadata with an object which has the name of the property and then sort out the properties themselves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, true enough. sounds like that’s boilerplate every metadata producer would likely need.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, which is what motivated the previous design. I could imagine with this proposal there being a common library in the ecosystem for doing this (I would probably write it myself tbqh) so if the complex design can't get consensus, I believe this is a reasonable but unfortunate alternative.

README.md Outdated Show resolved Hide resolved
@pzuraq pzuraq force-pushed the simpler-metadata branch from cb07f6c to a54495a Compare March 26, 2022 14:55
Copy link
Member

@Jack-Works Jack-Works left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel too good about this change. I think we should at least keep the symbol keyed part, instead of an array.

@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 26, 2022

@Jack-Works it would be pretty trivial to add a key to metadata kept this way, you would do something like:

const MY_META = Symbol();

function dec(_, context) {
  context.addMetadata({
    [MY_META]: true
  });
}

class C {
  @dec m() {}
}

const myMeta = C[Symbol.metadata].filter((m) => m && m[MY_META]);

You could even do it without symbols this way, using a WeakSet:

const MY_META = new WeakSet();

function dec(_, context) {
  const meta = {};

  MY_META.add(meta);
  
  context.addMetadata(meta);
}

class C {
  @dec m() {}
}

const myMeta = C[Symbol.metadata].filter((m) => MY_META.has(m));

@Jack-Works
Copy link
Member

const MY_META = Symbol();

function dec(_, context) {
  context.addMetadata(MY_META, true);
}

class C {
  @dec m() {}
}

const myMeta = C[Symbol.metadata][MY_META];

I prefer this. It doesn't feel right that I need to iterate all metadata to find the one I'm needed to.

@Jack-Works
Copy link
Member

And compared to #452 which is a small change, this is a big API change of the proposal, is it too hurry to make the decision? (it's only less than 3 days until the meeting)

@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 26, 2022

@Jack-Works the point of this proposal is to address the concerns raised by @syg and others about the metadata format being too complex. Without further input, it's difficult to know where the line is drawn for "too complex", so I'm going to keep it as simple as possible.

Agreed that this is too large of a change to make so close to the plenary, I did not intend to merge this prior to then and still intend to propose for stage 3 with the current metadata format. I'm putting this proposal up preemptively so that members of the committee understand the alternatives and can review them prior to that decision.

```

In addition, if two public class elements exist on the same class with the same name, then metadata defined on the second element will overwrite metadata defined on the element.
Metadata values which are added this way are then exposed in an array on the `Symbol.metadata` property of the value being decorated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like its not so much the "value being decorated", rather, the current class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is meant to describe the paradigm, in the future it might refer to the function being decorated, etc. Methods are part of the class, so it made sense to me that the class is the overall “value” being decorated.

Copy link
Contributor

@senocular senocular Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While methods are part of the class, when it comes to method decorators they're also a distinct value on their own and are the value "being decorated" - at least thats what the context is telling us. I would think there's got to be a better way to word this to make that distinction.

Also, where is the line drawn? Given future extensions, where would metadata for these decorators defined?

class C {
    field = @dec function expression(){}
    method(@dec parameter){}
}

Copy link
Collaborator Author

@pzuraq pzuraq Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fields are also a distinct value, yet have no way of being represented other than the class itself. So this interpretation of a “value” is more consistent in that sense, all class element metadata ends up on the class itself.

Parameter decorators do need to be considered, though they are out of scope for this proposal. I think it would make sense to add them to the methods/functions they are decorating. One possibility here would be to have method metadata be on the method, and then also be added to the class for convenience.

My main concern with having method metadata solely be on the method is the complexity it would introduce for getters/setters. However, I suppose it would be the “simplest” solution, metadata ends up on the decorated value (or nearest parent, in the case of fields and parameters) in a simple array, no other complexities, and decorator authors have to figure out the details.

@pzuraq
Copy link
Collaborator Author

pzuraq commented Jun 13, 2023

Closing as this has been superceded by: https://github.com/tc39/proposal-decorator-metadata

@pzuraq pzuraq closed this Jun 13, 2023
@ljharb ljharb deleted the simpler-metadata branch June 13, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants