pzuraq / macro-decorators

Decorators for getter/setter macros!
https://pzuraq.github.io/macro-decorators/
MIT License
62 stars 10 forks source link

@and, @or and some other decorators don't work with nested properties #11

Closed andreyfel closed 4 years ago

andreyfel commented 4 years ago

I've tried to use this package to replace standard ember computed macros and found an issue with @or decorator. This code just doesn't work:

@or('foo.bar', 'foo.baz')

Basically it is a very simple issue: getPaths should use getPath instead of obj[p].

Another incompatibility with ember computed macros is using {}. The above example could be rewritten as:

@or('foo.{bar,baz}')

and it would just work with ember's or macro. With macro-decorators it won't work. This syntax is kind of neat, so, it would be nice if this package could support it as well.

There is an open pr #10 which would conflict with a fix for this issue.

pzuraq commented 4 years ago

The path issue is a bug, a fix would be appreciated! We can merge that before #10 and rebase that PR, since it needs a bit of extra work.

For the expansion syntax, how would that work out in the general case, if a user is writing a macro using macro? I think it's important that the expansion syntax results in a deterministic argument order, so you could for instance map:

@customMacro('foo.{bar,baz}', 'qux', 'quux.{bar,baz}')

// to
@customMacro('foo.bar', 'foo.baz', 'qux', 'quux.bar', 'quux.baz')

I think if we can make sure this covers all cases, then I'm ok with adding it. I'm not personally a fan, but I realize the value for the time being during transition. It does make things harder to type, but these type of path lookups are hard anyways.

andreyfel commented 4 years ago

Created a pr #12 to fix the 1st issue.

For expansion syntax, I believe the behavior should be the same as in Ember. I'm not 100% how it works there but your description of the logic seems reasonable to me.

pzuraq commented 4 years ago

I'm not sure the syntax in Ember was ever fully rationalized 😄 at least, not in terms of passing dependencies to the macros. It made sense as a micro syntax for specifying dependencies, but that's all I know for sure.