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

Meta getProperty mutates underlying storage #40

Open
danielgtaylor opened this issue Jun 1, 2015 · 1 comment
Open

Meta getProperty mutates underlying storage #40

danielgtaylor opened this issue Jun 1, 2015 · 1 comment
Labels
Milestone

Comments

@danielgtaylor
Copy link
Contributor

This leads to non-obvious behavior which I think we need to properly define or change. Here is the implementation as it stands:

pimitives.es6#34-40

class Meta {
  getProperty(name, value) {
    if (!this.meta[name]) {
      this.meta[name] = registry.toType(value);
    }

    return this.meta[name];
  }
}

Here is the surprising part:

const element = new ElementType();

console.log(element.toCompactRefract());
// => ['element', {}, {}, null]

// Access the non-existing name property
element.name;

// What? Now there is a meta name for some reason!?
console.log(element.toCompactRefract());
// => ['element', {name: ''}, {}, null]

// Try to get something from meta with a default if not found
element.getProperty('foo', 'bar');

// What? Now my default value is getting serialized!
console.log(element.toCompactRefract());
// => ['element', {name: '', foo: 'bar'}, {}, null]

I'm sorry I glossed over this in the initial review! I believe the code is written this way to cache the conversion to refract element classes for the default value, but I think we need to either do that conversion each time or keep a separate cache that doesn't mutate the original meta values..

Thoughts @smizell?

@smizell
Copy link
Contributor

smizell commented Jun 4, 2015

@danielgtaylor Great find here. I think you're right. I'll be thinking on this as I do not think what I currently wrote is helpful to the user. I also think I should have made getProperty a private method.

I'll be thinking on this. I'm working on a proposal for Minim that I'll be running by you soon.

@smizell smizell added the bug label Jun 4, 2015
@smizell smizell added this to the Minim 1.0 milestone Jun 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants