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

Add option to use dot attribute access #44

Closed
beasteers opened this issue Aug 24, 2021 · 9 comments
Closed

Add option to use dot attribute access #44

beasteers opened this issue Aug 24, 2021 · 9 comments
Labels
design An issue regarding the user experience enhancement New feature or request implemented This feature is implemented in `develop` and ready for testing
Milestone

Comments

@beasteers
Copy link

beasteers commented Aug 24, 2021

Hi! I just want to say that I love this package and it's super useful for things like offloading dashboard search and filtering logic to config files so thank you!

My only hangup is that you have to do y of x of config instead of config.x.y which to me feels a bit confusing and counter-intuitive. And it makes it harder to teach others about how to use the expressions because it goes against how you access data in most other languages.

I wonder if it'd be possible to add a filter compile option that allows you to enable dot attribute access? That way we could keep the default functionality, but allow for a more familiar config.x.y to access attributes (instead of key literals) if a user prefers.

A headstart on a solution?

I'm not very familiar with language grammar definitions so I'm sure this isn't all right, but I'm hoping it's in the right direction.

Add ['\\.', 'return ".";'], after this line?
https://github.com/m93a/filtrex/blob/20a016d5c13527047520de4573eaeda98a49e69f/src/generateParser.js#L39

Replace this regex with: '[a-zA-Z$_][a-zA-Z0-9$_]*'
https://github.com/m93a/filtrex/blob/20a016d5c13527047520de4573eaeda98a49e69f/src/generateParser.js#L44

Add ['left', '.'], after
https://github.com/m93a/filtrex/blob/20a016d5c13527047520de4573eaeda98a49e69f/src/generateParser.js#L80

Add ['e . SYMBOL', code(['prop(', 1, ',', 3, ')'])], (I think?)
https://github.com/m93a/filtrex/blob/20a016d5c13527047520de4573eaeda98a49e69f/src/generateParser.js#L111

@bartbutenaers
Copy link

Hi @beasteers,
I think we have requested at the same day the same feature :-)
But our approach is completely different, as you can see in my issue.
Hopefully @m93a can find some spare time to have a look at both our proposals.
Kind regards,
Bart

@cshaa
Copy link
Owner

cshaa commented Sep 6, 2021

Hey @beasteers and @bartbutenaers!
Thank you both for your time, I'm looking forward to resolving these issues with you! I have my Bachelor's final exam on the 14th of September, after that I will finally have time to look into them. I already have some code commited but not yet released, your suggestions could make it into the v3 release :D

Regarding this issue specifically: since names containing a dot, like config.x.y, are already valid prop names, the problem of interpreting them as deep accessors can be already be solved in the current version of filtrex by making a custom prop function. However, it should at least be listed as a tutorial in the README, or better, be a “switch” that the developer can just turn on.

@bartbutenaers
Copy link

Hi @m93a,
Nice to hear from you!
Good luck with your final exam !!!!!!!!!!!!!!
Bart

@beasteers
Copy link
Author

That's so funny lol! I'm glad other people are thinking about this and that it seems like a solution is already quite feasible.

Good luck on your exam as well (today!!) I hope it goes/went well.

So just to document, it looks like this is the most basic solution here:

compileExpression(expression, {
    customProp: (key, get, data) => key.split('.').reduce((o, k) => o && o[k], data)
})

Here's a quick and dirty codepen to demo that it does indeed work https://codepen.io/beasteers/pen/WNOZodR?editors=1010

Thanks for the tip that this was possible! I agree that adding a util/flag is probably best (esp. considering you're doing some prototype-based attribute sanitizing and whatnot), but this is good enough for me for now esp. since I'm the only one controlling the queries atm

@cshaa
Copy link
Owner

cshaa commented Sep 17, 2021

Hey Bea,
I'm glad to hear you figured it out! :)
Thanks for the prototype, I'll add some sanitization and include it in v3. Hopefully, I'll manage to publish v3 (or at least a release candidate for it) on NPM during this weekend.

(PS: The finals were a success! Thank y'all for wishing me luck!)

@cshaa cshaa mentioned this issue Sep 17, 2021
15 tasks
@cshaa cshaa added design An issue regarding the user experience enhancement New feature or request labels Sep 17, 2021
cshaa added a commit that referenced this issue Sep 22, 2021
@cshaa cshaa added the implemented This feature is implemented in `develop` and ready for testing label Sep 23, 2021
@cshaa
Copy link
Owner

cshaa commented Sep 23, 2021

Published as 3.0.0-rc10 on NPM. Be sure to try it out and let me know what you think about it! 😁

The entire changelog is listed in #49 for the time being. I will add it to a more visible place before I publish 3.0.0 as stable.

To use the dot accessor, you'd write something like this:

import {
  compileExpression,
  useDotAccessOperator
} from 'filtrex'

const expr = "foo.bar"

const fn = compileExpression(expr, {
  customProp: useDotAccessOperator
});

fn({ foo: { bar: 42 } }) // → 42

And a more complete feature description can be seen here.

@beasteers
Copy link
Author

beasteers commented Sep 23, 2021

This is great thanks so much! One more request - could we have an option where it won't throw an error, it will just return undefined (even for a.b.c.d if a or b is undefined)? That way you can do a.b.c.d and search == a.b.c.d and it won't freak out if that part of the data doesn't exist. (Or if you have other ideas for resilient queries for flexible data?)

Currently I'm using this to query metadata across sensors and the data structure isn't always consistent.

export function useDotAccessOperator(name, get, obj, type) {
    // ignore dots inside escaped symbol
    if (type === 'single-quoted')
        return get(name)


    const parts = name.split('.')

    for (const propertyName of parts) {
        if (hasOwnProperty(obj ?? {}, propertyName)) {
            obj = obj[propertyName]
        } else {
            obj = undefined;
            break;
        }
    }

    return obj
}

Edit:

I guess this would be another way to define it

import UnknownPropertyError from 'filtrex.js/errors';

export function useDotAccessOperatorFallback(name, get, obj, type) {
    try {
        return useDotAccessOperator(name, get, obj, type);
    } catch (e) {
        if(e instanceof UnknownPropertyError) { return undefined; } 
        throw e;
    }
}

I can always copy this into code that I need it in, but I just figured that it seems like a slightly more useful form.

@cshaa
Copy link
Owner

cshaa commented Sep 23, 2021

You're right that optional chaining is something that could be useful for everybody, I've added it as useOptionalChaining.

Since there is no straightforward way to combine useOptionalChaining with useDotAccessOperator, I've also added useDotAccessOperatorAndOptionalChaining (quite a mouthful, huh?), which is exactly what you wanted :)
Published as 3.0.0-rc11.

If I should add more predefined prop functions, I'll probably think of some plugin system where things can be combined (similar to the way Rollup and Babel handle plugins), but for now this seems good enough.

@bartbutenaers
Copy link

@m93a,
This feature works fine for me. Thanks!!
Bart

@cshaa cshaa closed this as completed Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design An issue regarding the user experience enhancement New feature or request implemented This feature is implemented in `develop` and ready for testing
Projects
None yet
Development

No branches or pull requests

3 participants