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

Allow use of concise query string #1

Merged
merged 12 commits into from
Oct 4, 2018
Merged

Conversation

busterc
Copy link
Contributor

@busterc busterc commented Sep 28, 2018

These updates allow using a concise query string. Passing a query object
is still supported for backwards compatability, however, reference to using
the underlying object has been removed from the README to avoid creating
overly verbose usage documentation.

These updates allow using a concise query `string`. Passing a query `object`
is still supported for backwards compatability, however, reference to using
the underlying object has been removed from the README to avoid creating
overly verbose usage documentation.
@bmullan91
Copy link
Owner

@busterc This is great, thanks for the PR!

I noticed one issue - the string is space sensitive. This will likely trip a lot of people up, especially if they're coming from graphql which is space insensitive.

Eg: The following test would fail

const objectql = require('objectql');
const source = {
  a: 'a',
  b: {
    c: 'c'
  }
};
const query = '{b{c}}';
const actual = objectql(source, query);
const expected = {
  b: {
    c: 'c'
  }
};

Now '{a{b{c}}}' _like_ querystrings parse properly; an existing
test was modified for validating.
Copy link
Owner

@bmullan91 bmullan91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few small things - apart from that this is good to go 🎉

test/querystring-implementation.js Show resolved Hide resolved
README.md Outdated
@@ -37,7 +37,7 @@ objectql({ a: 'a', b: 'b' }, query); // returns { a: 'a' }
objectql([{ a: 'a', b: 'b' }, { b: 'b', c: 'c' }], query); // returns [{ a: 'a' }, {}]
```

__query__ must be an object, and only the keys where the value is `true` will be used. If an object is used as a key's value it will apply that object as the `query` param to that part of the source object recursively.
__query__ must be a concise string of key names, like a 'simple' graphql query.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you've updated all the docs to use the new style - but we should keep some documentation describing the object way.

What about:

__query__ Can be either a string or an object, see below for examples.

Then in the // valid query key values docs, we have examples of each query in both string + object form - the rest of the examples can be updated to the new string form like you've done.

objectql(source, { a: true }); // returns { a: 'a' }
objectql(source, { b: true }); // returns { b: { c: 'c', d: 'd' } }
objectql(source, { a: true, b: { c: true } }); // returns { a: 'a', b: { c: 'c' } }
objectql(source, '{ a }'); // returns { a: 'a' }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's where we should show how to do the same query as an object / string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this covers the bases: 1a8f088

README.md Outdated
```

## examples

The query object follows a similar pattern to a 'simple' graphql query, for each key in the query `objectql` will pick the matching key from the source object when the query value is `true`.
The query object follows a similar pattern to a 'simple' graphql query, for each key in the query `objectql` will pick the matching key from the source object.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding:

... In the examples below we'll be using the string graphql-like queries.

index.js Outdated
.replace(/" }/g, '":true }')
);
} catch (error) {
return null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should log then re-throw the error - these errors will typically occur at parse time and should be fixed or it will lead to other issues.

catch (error) {
  console.error(`Invalid objectql query: ${query}`);
  throw error;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set it to return null to avoid a definite API breaking change. With the current v1 API any !isObject(query) simply returns the source unaltered: https://github.com/bmullan91/objectql/blob/master/index.js#L4-L5

Are you certain you want to throw here?

If so, how do you want to update README doc for:
// invalid queries

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is a tricky one... With the previous API, the worst you could do is have a coarse query, since the interpreter would catch invalid objects.

With the string queries it's much more likely a developer could have an invalid query - and if we only logged an error/warning it could just get ignored, defeating the purpose.

If so, how do you want to update README doc for:
// invalid queries

We'd keep the same behaviour for queries that aren't objects or strings.

For invalid string query:

// remove...
objectql(source, ''); // returns source

// add...
// invalid string queries
objectql(source, '') // throws
objectql(source, '{ hello }}') // throws

I'm open to other ideas though!

index.js Outdated
}

// eslint-disable-next-line no-param-reassign
query = parse(query);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's store this in a separate const and remove the eslint-disable-next-line no-param-reassign. Naming things is tricky, what about:

const parsedQuery = parse(query);

Moving the empty `string` test to `querystring-implementation.js` tests.
Don't attempt to parse any non-string query.
Catches, logs to console.error then re-throws an unparseable query.
Throws:
- empty string
- malformed query
Updated explanation of query.
Added explanation about using query string in examples.
Shows how to query with object or string.
Documents invalid query string expectations.
@bmullan91 bmullan91 merged commit 8625a1b into bmullan91:master Oct 4, 2018
@bmullan91
Copy link
Owner

Thanks again @busterc - v2 is now published https://www.npmjs.com/package/objectql.

What kind of things are you building with it out of curiosity?

@busterc
Copy link
Contributor Author

busterc commented Oct 5, 2018

Thanks @bmullan91 ... for now I'm going to use it for some miscellaneous internal tooling. I wanted an _.pick() that could handle collections (top level/nested) and use graphql'ish syntax. I almost just used objectql as a dep, but figured you might appreciate baking the concise query string feature into objectql.

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants