feat: Column.excludeFieldFromQuery, exclude field but keep fields array #1217

Harsgalt86 · 2023-11-24T06:32:12Z

-- Situation --
From the GraphQL, we may have a fragment that is as such

fragment user on User{
   firstName
   lastName
   fullName @client
}

We want the firstName and lastName from the server, but fullName will be populated locally through policy
https://www.apollographql.com/docs/react/local-state/managing-state-with-field-policies/#querying

In the table, we want the column to display the fullname

{
    id: "full-name",
    field: "fullName",
    fields: ["firstName", "lastName"],
}

-- Problem --
The generated query by slickgrid attempts to request all 3 properties firstName, lastName and fullName.
We want to only request firstName and lastName

-- Solution --
There already exists a column propety excludeFromQuery that excludes the entire columnDef from the buildQuery. This PR introduces another property excludeFieldFromQuery which excludes column.field but still adds column.fields to the query

The result is the following column definition

{
    id: "full-name",
    field: "fullName",
    fields: ["firstName", "lastName"],
    excludeFieldFromQuery: true,
}

Side note: The queryField property basically solves the same underlying problem but specifically for filtering and sorting.

…lue in the parameter signature

…ersal

codecov · 2023-11-24T06:37:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aa1939c) 100.00% compared to head (f923994) 100.00%.
Report is 61 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #1217   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          245       245           
  Lines        17246     17277   +31     
  Branches      6232      6247   +15     
=========================================
+ Hits         17246     17277   +31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ghiscoding · 2023-11-24T06:47:12Z

hmm I'm wondering if it wouldn't be better to do the inverse instead? For example keep using excludeFromQuery but have another inversed flag for the fields includeFieldsInQuery. I had this idea just because it might be confusing to have excludeFromQuery and excludeFieldFromQuery, they seem very similar... your new flag is not that bad though, I'm just thinking what would be less confusing if possible, though I'm not crazy about the name I gave includeFieldsInQuery, it's just a thought. It's bedtime for me now, so think about it and I'll reply tomorrow

Harsgalt86 · 2023-11-24T07:01:02Z

they seem very similar...

From how I've been using TypeScript, interfaces and documentation, I think it's handy they similar as they appear close together when looking for similar behaviours. eg all the other exclude* options on the column are close together (excludeFromExport, excludeFromGridMenu, excludeFromHeaderMenu etc). Having said that queryField is functionally similar and not anywhere near them...

However, one consideration is that excludeFromQuery and excludeFieldFromQuery are logically consistent. For example:

{
    excludeFromQuery: true, 
    excludeFieldFromQuery: true,
}

=> The entire column, and by extension, the field, are excluded from the query. The behaviour is consistent with the settings

Conversely

{
    excludeFromQuery: true, 
    includeFieldsInQuery: true,
}

is not logically consistent as the entire column is excluded and the fields are NOT included. The behaviour is not consistent with specifying includeFieldsInQuery: true

Would you ever put the two options together? Probably not... but at least with the former the behaviour isn't ambiguous? I'll leave it to you to decide :)

Harsgalt86 · 2023-11-24T08:01:49Z

Another property name could be queryOnlyUsesFields?

ghiscoding · 2023-11-24T14:29:41Z

is not logically consistent as the entire column is excluded and the fields are NOT included. The behaviour is not consistent with specifying includeFieldsInQuery: true

Would you ever put the two options together? Probably not... but at least with the former the behaviour isn't ambiguous? I'll leave it to you to decide :)

Yes that was my intention, I thought of that approach because that is how a TypeScript tsconfig.json works, you can exclude but also include and I'm pretty sure include has priority over the exclude but I could be wrong too, anyway that is how I got the idea

Another property name could be queryOnlyUsesFields?

hmm not crazy about the name either haha... but in that case then we might as well keep your first approach but I'm wondering if we should have 3 options then? excludeFromQuery, excludeFieldFromQuery and excludeFieldsFromQuery, however in that case you might think, why declare the fields in the first place? ... anyway, we might as well go with your current approach and be done with it :)

Let me know if you have any other suggestions or if I should go ahead with merging?

ghiscoding · 2023-11-24T14:35:34Z

packages/common/src/interfaces/column.interface.ts

@@ -108,6 +108,9 @@ export interface Column<T = any> {
  /** Default to false, which leads to exclude the column title from the Grid Menu. */
  excludeFromGridMenu?: boolean;

+  /** Defaults to false, which leads to exclude the field property, but including the fields property, from the query (typically a backend service query) */


can you use backticks on the column options, when we use that it shows them as code in the editor as well

/** ... which leads to exclude the `field` property, but including the `fields` property

Could you also update the next option I created as well. I started using this approach not long ago, I didn't know VSCode also shows code this way :) see example below, thought it's hard to see, the field has a color background

ghiscoding · 2023-11-24T14:39:34Z

packages/graphql/src/services/graphql.service.ts

@@ -98,7 +98,9 @@ export class GraphqlService implements BackendService {
    const columnIds: string[] = [];
    if (columnDefinitions && Array.isArray(columnDefinitions)) {
      for (const column of columnDefinitions) {
-        columnIds.push(column.field);
+        if (!column.excludeFieldFromQuery) {
+          columnIds.push(column.field);


could you apply the same logic in the OData Service as well, they should have the same feature as much as possible

ghiscoding · 2023-11-24T22:48:44Z

@Harsgalt86 I want to release a new version this weekend (Saturday or Sunday), I would rather wait for this PR to be updated but I don't want to wait too long 😉

Harsgalt86 · 2023-11-25T11:38:55Z

Sorry, only just saw this. I did look at updating the odata service before opening the PR but didn't see any code directly adding 'fields' in the buildQuery function nor were there any unit tests directly testing the same way the graphql.spec was. I'll take another look with fresh eyes, hopefully tomorrow morning if I get some spare time, otherwise I will on Monday. It would be good to get this merged in for the next release, but don't want to hold you up either :)

…

On Sat, 25 Nov 2023, 9:18 am Ghislain B., ***@***.***> wrote: @Harsgalt86 <https://github.com/Harsgalt86> I want to release a new version this weekend (Saturday or Sunday), I would rather wait for this PR to be updated but I don't want to wait too long 😉 — Reply to this email directly, view it on GitHub <#1217 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BCEFSVHXK3A472AMNWHVJVDYGEP5PAVCNFSM6AAAAAA7YVGLSCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGEZDEMJXHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ghiscoding · 2023-11-25T17:12:41Z

I did look at updating the odata service before opening the PR but didn't see any code directly adding 'fields' in the buildQuery function nor were there any unit tests directly testing the same way the graphql.spec was.

oh right I just remembered now that it's only for GraphQL, we didn't need it for OData because most of the time in OData and REST API you pull all the fields at once, that's kinda why we migrated to GraphQL.

So it's all good, if you could just do the other little changes I asked, that would be great. I

Harsgalt86 · 2023-11-25T21:26:31Z

All done.

Ronald Van Ryswyk added 4 commits October 6, 2023 11:21

Pass in undefined (instead of null) so TypeScript uses the default va…

6ff81d4

…lue in the parameter signature

Merge branch 'master' of https://github.com/ghiscoding/slickgrid-univ…

1f58c52

…ersal

Merge branch 'master' of https://github.com/ghiscoding/slickgrid-univ…

5dff90b

…ersal

Implement excludeFieldFromQuery flag

4ec4c4e

ghiscoding requested changes Nov 24, 2023

View reviewed changes

ghiscoding changed the title ~~Feat: Column.excludeFieldFromQuery => Exclude field from query, but fields are still sent on the query~~ feat: Column.excludeFieldFromQuery, exclude field but keep fields array Nov 24, 2023

Use backticks for properties in JsDoc

f923994

ghiscoding approved these changes Nov 25, 2023

View reviewed changes

ghiscoding merged commit 85cc514 into ghiscoding:master Nov 25, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Column.excludeFieldFromQuery, exclude field but keep fields array #1217

feat: Column.excludeFieldFromQuery, exclude field but keep fields array #1217

Harsgalt86 commented Nov 24, 2023 •

edited

Loading

codecov bot commented Nov 24, 2023 •

edited

Loading

ghiscoding commented Nov 24, 2023

Harsgalt86 commented Nov 24, 2023 •

edited

Loading

Harsgalt86 commented Nov 24, 2023

ghiscoding commented Nov 24, 2023

ghiscoding Nov 24, 2023

ghiscoding Nov 24, 2023

ghiscoding commented Nov 24, 2023

Harsgalt86 commented Nov 25, 2023 via email

ghiscoding commented Nov 25, 2023

Harsgalt86 commented Nov 25, 2023

feat: Column.excludeFieldFromQuery, exclude field but keep fields array #1217

feat: Column.excludeFieldFromQuery, exclude field but keep fields array #1217

Conversation

Harsgalt86 commented Nov 24, 2023 • edited Loading

codecov bot commented Nov 24, 2023 • edited Loading

Codecov Report

ghiscoding commented Nov 24, 2023

Harsgalt86 commented Nov 24, 2023 • edited Loading

Harsgalt86 commented Nov 24, 2023

ghiscoding commented Nov 24, 2023

ghiscoding Nov 24, 2023

Choose a reason for hiding this comment

ghiscoding Nov 24, 2023

Choose a reason for hiding this comment

ghiscoding commented Nov 24, 2023

Harsgalt86 commented Nov 25, 2023 via email

ghiscoding commented Nov 25, 2023

Harsgalt86 commented Nov 25, 2023

Harsgalt86 commented Nov 24, 2023 •

edited

Loading

codecov bot commented Nov 24, 2023 •

edited

Loading

Harsgalt86 commented Nov 24, 2023 •

edited

Loading