Comments (12)
The whole defaultValue thing is a bit of a hack (as you can probably see); I'd consider a PR that improved it somehow - I'm not sure what you have in mind exactly.
We definitely need a better flow for replacement plugins; at the moment I'd recommend copying and pasting the entire thing into a new plugin, editing as you see fit, and then when you load the plugins inform the system to NOT load the original plugin. It would be nice if we came up with some kind of replaces: PgConnectionArgOrderBy
or similar configuration thing, but I can imagine this being problematic... Needs more thought.
would you rather see an orderByMany plugin implemented as a PR or maintained separately?
I would like to keep the core as lean as possible; as much as possible additional plugins should be in their own npm modules/GitHub repos. If it turns out that a majority of people want a particular plugin in core we will look at merging it in once it's stable and proven, probably involving a major version bump.
from graphile-engine.
I'm not big on the replacement plugins approach.. once you start removing and reimplementing core plugins like OrderBy, it starts to feel like maintaining a fork instead of a plugin. So if possible, I'd like to find a way for PgConnectionArgOrderBy and PgConnectionArgOrderByMany to coexist, just like my custom filter plugin and the core condition plugin do.
My main question is.. what purpose is the defaultValue actually serving? When I drop it from the PgConnectionArgOrderBy and run the full test suite, the only failure is the postgraphile-core schema test where the default values get stripped out on all the OrderBy types (obviously). But I presume it's serving some other purpose that isn't covered by the current test suite.
from graphile-engine.
If memory serves, it's because otherwise the cursors don't make sense because the results from the database aren't ordered so Postgres can return them in any order. But it was quite a few months ago now so it may have been for a different reason.
from graphile-engine.
Ahhhh; no it was to maintain as much compatibility with v3 as possible:
Basically I tried to minimise all the diffs in all the tests. e.g.:
https://github.com/postgraphql/postgraphql/pull/506/files#diff-c12cf6b4454a6f09ca43c15fa9936592 (click to show diff)
from graphile-engine.
Gotcha. If minimizing those diffs is still a goal for v4, would you consider a PR that factored out the defaultValue assignment into a PgConnectionArgOrderByDefaultValue core plugin? With that in place, I could drop PgConnectionArgOrderByDefaultValue, keep PgConnectionArgOrderBy, and append a new PgConnectionArgOrderByMany plugin.
from graphile-engine.
Yes. But first; remove the default value logic and ensure that the queries generated still order by the primary key by default. I think they will because I'm pretty sure I added it as a fallback.
Use DEBUG="*:sql"
envvar to see SQL queries.
If they don't then we'll need to solve that before you can factor it out.
from graphile-engine.
It appears to still order by the primary key by default. Using postgraphile v4.0.0-alpha2.26 with just this line removed, the following query:
query {
allPosts {
nodes {
id
}
}
}
run against the forum_example
schema produces:
with __local_0__ as (
select to_json((__local_1__."id")) as "id"
from "forum_example"."post" as __local_1__
where (TRUE) and (TRUE)
order by __local_1__."id" ASC
), __local_2__ as (select json_agg(to_json(__local_0__)) as data from __local_0__) select coalesce((select __local_2__.data from __local_2__), '[]'::json) as "data"
This is where the fallback is implemented, correct?
from graphile-engine.
Great - I thought I had it handled but it always pays to check.
I really need to get around to tidying up the SQL at some point 😳
from graphile-engine.
After reading about List Input Coercion in the GraphQL spec, I realized that's it possible for a single orderBy
argument to support both single values and arrays, eliminating the need for a separate orderByMany
argument.
Technically, support for passing an array to orderBy
could be implemented as a non-breaking improvement, but it would cause a bunch of diffs in the schema tests.. (all instances of orderBy: TableOrderBy = PRIMARY_KEY_ASC
would become something like orderBy: [TableOrderBy!] = [PRIMARY_KEY_ASC]
) so that's not an option for v4.
I can think of a few ways to handle this:
- Replace the core OrderBy plugin with a non-core plugin that supports array inputs. This would definitely work, but users would have to invoke
replaceAllPlugins
, at least until areplacePlugins
function is added to graphile-build. It's also more of a maintenance burden. - Update the core OrderBy plugin to handle array inputs, but don't actually change the type of the orderBy argument. Then a simple non-core plugin could be written to allow array inputs, simply wrapping the orderBy types with
new GraphQLList(new GraphQLNonNull(...))
. - Update the core OrderBy plugin to handle array inputs, and allow setting the orderBy type with an option such as
enableOrderByMany
(defaulting to false).
Thoughts?
from graphile-engine.
Additional background here: graphql/graphql-relay-js#20 (comment)
from graphile-engine.
So long as none of people's existing queries will break then I don't see that as a breaking change even if the diffs change. I had noticed this behaviour of GraphQL before but had not realised it was deliberate - just thought it was a fluke! I should read the spec more often!
Basically if there's a query that's like this:
query Foo($orderBy: TableOrderBy) {
allFoos(orderBy: $orderBy) {
nodes { id }
}
}
And that will still execute when orderBy
changes from orderBy: TableOrderBy
to orderBy:[TableOrderBy!]
then I'm good with it.
I'm trying to keep diff changes to a minimum but there's no requirement that they be zero.
from graphile-engine.
Closed by #144
from graphile-engine.
Related Issues (20)
- [graphql-parse-resolve-info] Fix types or example HOT 3
- Custom functions for `nodeId` in `NodePlugin` HOT 2
- Add depth traversal to `simplifyParsedResolveInfoFragmentWithType` HOT 2
- No Way to Access Request From Resolvers? HOT 1
- [graphile-build]: Types for Build Don't Match Source HOT 4
- graphql-parse-resolve-info scalar fields HOT 2
- Pagination cursors not working for tables with at least one bigint component HOT 6
- Modify column SQL select HOT 1
- graphile/subscriptions-lds does not trigger on DELETE if subscription contains only edges
- Project depends on vulnerable version of jsonwebtoken HOT 2
- graphql-parse-resolve-info - What to do to get the same output like graphql-fields? HOT 5
- Don't silently swallow exceptions in resolveNode HOT 2
- alias a column at mutation time only? HOT 2
- Type with array property triggers error HOT 10
- Error using PG client: "We only support PG clients from a PG pool..." HOT 4
- pg-pubsub call to removeListener looks like it will not work HOT 3
- Provide a type guard to distinguish between ResolveTree and FieldsByTypeName HOT 2
- Multiple services resulting in duplicate codecs for simple types HOT 1
- Defining two types with the same schema should be allowed HOT 1
- multi tenancy and add schemas for watch HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from graphile-engine.