ericmorand / drupal-attribute

Apache License 2.0
4 stars 7 forks source link

Cannot access values directly #9

Open stevetweeddale opened 4 years ago

stevetweeddale commented 4 years ago

The Drupal documentation suggests that you should be able to access properties of an Attribute object in a twig template via either dot or bracket notation. But as this object is implemented as a Map, those properties don't exist.

I spent an afternoon messing about with Proxies and Reflect and managed to get something that essentially adds property accessors for the keys available in the Map. If you change the constructor to be like so:

class DrupalAttribute extends Map {
    constructor(it) {
        super(it);
        return new Proxy(this, {
            get(target, prop, receiver) {
                let value = Reflect.get(...arguments);
                return typeof value == 'function' ? value.bind(target) : target.get(prop);
            },
        });
    }
}

That works when running a test script in Node. I can access values in the Attributes object via attributes.id or attributes['id']. Pseudocode is "when someone tries to access a property, call .get to pull it from the map, unless that property is a function, in which case call it as normal".

However much to my disappointment this doesn't seem to work when used within Twig.js (which I suspect is the primary use case for this library?) as it would appear Twig.js does clever things based on type, rather than for example just dumbly executing attributes.id regardless. Specifically: if I wrap a plain object in a Proxy and pass that into a twig template, and use the proxy to log the prop that gets accessed, it is id and everything works. As soon as I wrap a Map object instead, then the property that gets accessed with attributes.id is actually getId. I haven't dug into the twig.js internals to figure that one out yet!

However, the obvious other option I can think of (instead of adding property access to Maps) would be to reimplement this whole class as a plain Object. Am I missing something, or could that be a legit option? I can't think of any of the features of Maps that would be deal-breakers in this case (Keys should always be alphanumeric right? Ordering of attributes shouldn't matter?). Any thoughts?

ericmorand commented 3 years ago

Interesting question, but first, sorry for the delay, I missed the notification.

This library has been tested using Twing. Twig.js, for all of its pros, isn't a perfect implementation of Twig specification. How it deals with attributes is unknown to me but I suspect it is not implementing the whole specification of Twig, which is quite complex:

https://github.com/twigphp/Twig/blob/f1e0ebc18a6b7e036a1dad8c6daf4fc967d9cc86/src/Extension/CoreExtension.php#L1378

With Twing, the following code output the expected result:

import {TwingEnvironment, TwingLoaderArray} from "twing";

const DrupalAttribute = require('drupal-attribute');

const attribute = new DrupalAttribute();

attribute.setAttribute('foo', 'bar');

const environment = new TwingEnvironment(new TwingLoaderArray({
  index: `{{ attribute.foo }}{{ attribute["foo"] }}`
}));

environment.loadTemplate('index').then((template) => {
  template.render({
    attribute
  }).then((output) => {
    console.warn(output); // barbar
  })
});

This said, using a plain object instead of a Map looks like a totally legit option to me.

I am migrating all my old JS libs to TypeScript so it may be the perfect moment to upgrade drupal-attribute to plain object + TyepScript.

JohnAlbin commented 2 years ago

This library has been tested using Twing.

If you only support Twing, can we add that to the README? :)

Earlier this year, I couldn't get Twing to work with Storybook, so I've been stuck using Twig.js for now. I'm seeing lots of Attribute test failures in both Twing and Twig.js, so I'm probably going to have to fork this repo to support both. I'll make sure I pass on the Twing-related fixes back here as PRs.

Thanks for all your work, @ericmorand!