Return All Inserted Rows in Many Create

[markhalonen]

Hey Tim, this PR seems to work. We'd likely want to add this to Many Update if this approach is correct

[markhalonen]

I've never touched a Postgraphile Plugin, I have no idea what I'm doing. I copied what they did at https://github.com/graphile-contrib/pg-many-to-many/blob/715e84fb3b021e00bbfc20031bd8a3c5e51c4050/src/createManyToManyConnectionType.js#L184 and it seems to work! Not sure what the null policy should be here...

[markhalonen]

@benjie This might be of interest? It seems Postgraphile does a lot of "pluralizing", is there a function I can use besides my naive strategy of appending an s? Thanks!

Also if you look below, does type: new GraphQLList(tableType) make sense?

[tjmoses]

Upon first glance, you'll need to integrate with the types I had and update those vs. using "any" types. Using "any" invalidates my effort and all the types I built (some are implied as any, which is ok for now).

For the names the inflection plugin/something similar should be used (it uses another hook). The entire plugin needs to use that vs. trying to hardcode the name in (even the mn prefix). I suppose this would be tied into plugin options as well. Another item that needs fixed on this library. Benjie made another plugin that replaces many names and tries to standardize on some of the naming, which is a great example for this.

[markhalonen]

Upon first glance, you'll need to integrate with the types I had and update those vs. using "any" types. Using "any" invalidates my effort and all the types I built (some are implied as any, which is ok for now).

Ok, my tsconfig must be different, it was complaining about implicit any. I'll look into adjusting that back.

For the names the inflection plugin/something similar should be used (it uses another hook). The entire plugin needs to use that vs. trying to hardcode the name in (even the mn prefix). Another item that needs fixed on this library. Benjie made another plugin that replaces many names and tries to standardize on some of the naming, which is a great example for this.

Ok, if the api is not going to change, I think we'll go forward to using this in production. I mostly would be concerned with GraphQL API having to change, and us having to migrate our frontend code...

[tjmoses]

If I'm not mistaken, changing the table names to all have an additional s will break my projects using this. So maybe we introduce options and for people that want that they can add the option. Also, there is still the return type issue that doesn't line up (the reason I didn't return rows before). You could always use git+{url}.git and your PR as your package if you wanted it in your project immediately (I do that all the time).

[markhalonen]

If I'm not mistaken, changing the table names to all have an additional s will break my projects using this.

Ok, yeah I can see why this is a breaking change. I am unsure of the utility of inserting multiple rows and only being able to query the first row that got inserted... perhaps do a major release of 2.0.0?

Also, there is still the return type issue that doesn't line up (the reason I didn't return rows before)

Are you sure it doesn't line up? Here https://github.com/tjmoses/postgraphile-plugin-batch-create-update-delete/pull/15/files#diff-d98a4086916d405fc31ca771c4284a92ead266cfa14b390fea7c28772ad3354bR160 I return a GraphQLList and it seems to work correctly when I test it...

You could always use git+{url}.git and your PR as your package if you wanted it in your project immediately (I do that all the time).

Yes, I will probably do this if it doesn't get released by early next week. No big deal!

[tjmoses]

You may be able to use the plugin I was referring for directly adding the s. Actually, the mn should imply many so an s isn't needed. People have their own ideas for naming, so this is where settings are handy.

I would like to hold off on an official v2 until the tests I wrote are integrated in with Circle CI, and additional features like the name changing are added.

For the return list I suppose GraphQLList is easy enough, but no I have not tested anything just glanced it over (seems ok, didn't see that earlier). If this is the only change and it works, def could do a v2.0.0-canary.0 version. Only worried that the new return value may break people's code bases since it was one item and is now a list.

[benjie]

Indeed any time you add a name to the GraphQL schema (type name, field name, argument name, directive name, enum name, etc) that name should come directly from an inflection function; you can read more about inflection here: https://www.graphile.org/postgraphile/inflection/

[s2mr]

I need this PR