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

Don't use an out parameter context.metadata #351

Closed
bergus opened this issue Dec 10, 2020 · 1 comment
Closed

Don't use an out parameter context.metadata #351

bergus opened this issue Dec 10, 2020 · 1 comment

Comments

@bergus
Copy link

bergus commented Dec 10, 2020

The proposal currently states that "Decorators can add metadata about class elements by adding a metadata property of the context object that is passed in to them."

@littledan justified this in a comment on the pull request:

I […] chose this somewhat funny-looking calling convention because it seemed the simplest all around:

  • returning a complex object would complicate the common case, requiring you to wrap it in another object, which seems unfortunate
  • making a method on the context object would require either adding an own method or making it an instance of a class, and either way adds considerable complexity (e.g., what happens if this escapes, and it gets called later?).

I feel the urge to disagree. Out parameters are really unconventional in JavaScript, and I would consider it bad style to introduce them here.

I'd like to propose another alternative: just annotate the returned object itself!

function annotate(metadata) {
  return val => {
    if (val[Symbol.metadata])
      Object.assign(val[Symbol.metadata], metadata);
    else
      val[Symbol.metadata] = Object(metadata);
    return val;
  }
}

This mostly has the advantage that it works out of the box, requiring special language support only for fields (whose annotations need to be moved onto the class somehow, somewhere). Multiple decorators on the same element can easily communicate (#328). Frameworks could use their own symbols for framework-specific metadata. I kinda expect that this will happen anyway because of its simplicity, so why not make this the one and only way to do annotations? Later, with the Reflect.metadata proposal, I would expect decorators to transition to using the Reflect methods on the returned value instead of installing properties.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 27, 2022

The metadata API has been updated since this suggestion, and I believe it is more intuitive/understandable than the original API so this may no longer be an issue. I'm going to close this issue, but feel free to reopen or make a new issue.

@pzuraq pzuraq closed this as completed Mar 27, 2022
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

No branches or pull requests

2 participants