Updating meta of proto object wrongly

[arpit-agarwal]

When we have a mutable value on proto object and update the same to save on self. It is updating the metadata value on proto object as well.

I do understand that I should clone the value before updating but still when we get values from proto ( not self) it should be cloned and passed to avoid accidental fail. I could have done thsi but this was @angular/ compiler code that was causing an issue.

I have froked the repo here https://github.com/arpit-agarwal/reflect-metadata and added a test-case which fails on your repo and works after my fix for cloning. If desired i may create a pull request.
MutableKeyValueDefinedOnPrototype

    it("MutableKeyValueDefinedOnPrototype", () => {
        let prototype = {};
        let obj = Object.create(prototype);
        Reflect.defineMetadata("key", ["value_proto"], prototype, undefined);
        let result = Reflect.getMetadata("key", obj, undefined);
        assert.equal(result[0], "value_proto");
        result.push("value_self");
        Reflect.defineMetadata("key", result , obj, undefined);
        let result_proto = Reflect.getMetadata("key", prototype, undefined);
        let result_self = Reflect.getMetadata("key", obj, undefined);
        assert.equal(result_proto.length, 1);
        assert.equal(result_self.length, 2);

    });

[DanielTWalther]

Hey @arpit-agarwal , your fork works wonderfully. And here I was getting frustrated, thinking my decorator functions were faulty. This is exactly the solution I needed for my decorator based object validator. I personally vote for merging this into the main repo.

[rbuckton]

I don't see this as a bug. Nothing about this API explicitly calls for immutability or cloning. You have the same issue if you do the following in JavaScript today:

class C {
  constructor() {
    this.array.push("value_self");
  }
}
C.prototype.array = ["value_proto"];

console.log(C.prototype.array); // ["value_proto"]
console.log(new C().array); // ["value_proto", "value_self"]
console.log(C.prototype.array); // ["value_proto", "value_self"]

Instead, its better to be more defensive when mutating any metadata entry:

// decorator
function example(target: any) {
  setExample(target, "value_proto");
}

// gets value of decorator on object
function getExample(target: any) {
  const array = Reflect.getMetadata("key", target);
  return array && array.slice(); // defensive get, creates a copy
}

// sets value of decorator on object
function setExample(target: any, value: string) {
  const data = [value];
  const previous = Reflect.getMetadata("key", target);
  const result = previous ? previous.concat(data) : data; // create a copy
  Reflect.defineMetadata("key", result, target); // defensive set
}

@example
class C {
  constructor() {
    setExample(this, "value_self");
  }
}

console.log(getExample(C.prototype)); // ["value_proto"]
console.log(getExample(new C()); // ["value_proto", "value_self"]
console.log(getExample(C.prototype)); // ["value_proto"]

[rbuckton]

@arpit-agarwal, if this is an issue in Angular, I would recommend you open an issue in http://github.com/angular/angular/ referencing this.

[DanielTWalther]

@rbuckton, let's assume I have (using typescript) multiple classes inheriting from one superclass, each having some properties decorated (e.g. for validation purposes), like so:

'use strict';
require('reflect-metadata');

const METADATAKEY: string = "key";

function applyDecorator(target: any, propertyName: string, type: string, value?: any) {
  let meta: any[] = Reflect.getMetadata(METADATAKEY, target);
  if (!meta) {
    meta = [];
  }
  meta.push({
    type: type,
    property: propertyName,
    value: value
  });
  Reflect.defineMetadata(METADATAKEY, meta, target);
}

// some decorators in the style of:
function ofType(value: any) {
  return function(target: any, propertyName: string) {
    applyDecorator(target, propertyName, 'ofType', value);
  }
}

function isDefined() {
  return function(target: any, propertyName: string) {
    applyDecorator(target, propertyName, 'defined');
  }
}

function deep() {
  return function(target: any, propertyName: string) {
    applyDecorator(target, propertyName, 'deep');
  }
}

class Utility {
  @isDefined()
  public id;
  public validate() {
    // validate current instance of this class, return potential errors
  }
  public save() {
    // save current instance to a database, returning success state
  }
  public serialize() {
    // return serialized form of current instance
  }
}

class Engine extends Utility {
  @ofType(Number)
  public horsepower;
  @ofType(Number)
  public displacement;
  public start() {
    // engine starts
  }
  // some more methods or properties
}

class Car extends Utility {
  @ofType(Engine)
  @deep()
  public engine: Engine;
  public start() {
    // car starts
  }
  public accelerate(rate: number) {
    // car accelerates
  }
  // some more methods or properties
}

console.log(Reflect.getMetadata(METADATAKEY, new Utility()));

Now, the decorators of each inheriting class apply to Utility; expecting to get:

[ { type: 'defined', property: 'id', value: undefined } ]

Actually getting:

[ { type: 'defined', property: 'id', value: undefined },
  { type: 'ofType',
    property: 'horsepower',
    value: [Function: Number] },
  { type: 'ofType',
    property: 'displacement',
    value: [Function: Number] },
  { type: 'deep', property: 'engine', value: undefined },
  { type: 'ofType', property: 'engine', value: [Function: Engine] } ]

Is that working as intended and I am using Reflect.defineMetadata(key, target) in the wrong way?

[rbuckton]

As I mentioned above, don't use push but instead use concat to create a fresh copy of the array.