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.

Sign up for GitHub

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

fix[gen1][core] ENG-7841 change to queryParams #3787

Merged
merged 12 commits into from
Dec 23, 2024
Merged

fix[gen1][core] ENG-7841 change to queryParams #3787

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
934f83f
fix[gen1][core] ENG-7841 change to queryParams
Dec 17, 2024
a6995c0
fix[gen1][core] ENG-7841 remove console.log()
Dec 17, 2024
42a258b
fix[gen1][core] ENG-7841 change to tests
Dec 18, 2024
0356239
Merge branch 'main' into ENG-7841
Dec 18, 2024
89107d3
fix[changeset] ENG-7841 changeset added
Dec 18, 2024
a045275
fix[changeset] ENG-7841 change to tests
Dec 19, 2024
db72aed
Merge branch 'main' into ENG-7841
Dec 19, 2024
620430d
fix[changeset] ENG-7841 change to changesets
Dec 19, 2024
6741f22
Update .changeset/famous-pants-agree.md
clyde-builderio Dec 20, 2024
cc4168a
fix[gen1][core] ENG-7841 change to tests for noTraverse
Dec 20, 2024
e17d580
fix[gen1][core] ENG-7841 change to tests names for noTraverse
Dec 20, 2024
c11643b
fix[gen1][core] ENG-7841 set noTraverse default value in get()
Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/core/src/builder.class.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2640,7 +2640,9 @@ export class Builder {
const isApiCallForCodegenOrQuery = isApiCallForCodegen || this.apiEndpoint === 'query';

if (this.apiEndpoint === 'content') {
queryParams.enrich = true;
queryParams.noTraverse = queue[0].limit !== 1;
queryParams.includeRefs = true;

Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this logic before the following code blocks:

  • builder/packages/core/src/builder.class.ts

    Lines 2604 to 2616 in 1908d27

    for (const key of properties) {
    const value = options[key];
    if (value !== undefined) {
    if (this.apiEndpoint === 'query') {
    queryParams.options = queryParams.options || {};
    queryParams.options[options.key!] = queryParams.options[options.key!] || {};
    queryParams.options[options.key!][key] = JSON.stringify(value);
    } else {
    queryParams[key] = JSON.stringify(value);
    }
    }
    }
    }

  • builder/packages/core/src/builder.class.ts

    Lines 2627 to 2630 in 1908d27

    if (this.overrideParams) {
    const params = omit(QueryString.parse(this.overrideParams), 'apiKey');
    assign(queryParams, params);
    }

These are two ways customers can override properties, so they need to exist after you set the noTraverse and includeRefs defaults.

I also just noticed these two noTraverse settings:

builder/packages/core/src/builder.class.ts

Lines 2495 to 2497 in 1908d27

if ('noTraverse' in queue[0]) {
queryParams.noTraverse = queue[0].noTraverse;
}

builder/packages/core/src/builder.class.ts

Lines 2863 to 2867 in 1908d27

// Set noTraverse=true if NOT already passed by user, for query performance
if (!('noTraverse' in options)) {
options.noTraverse = true;
}

This means that noTraverse is already handled (set to true in getAll() which has limit > 1. So you don't need to set noTraverse, you only need to add the includeRefs = true.

TLDR:

  • remove noTraverse default
  • move the includeRefs default higher up so it can still be overridden by customer
  • add some tests that show:
    • default value of includeRefs is true
    • if customer provides includeRefs: false to builder.get() or builder.getAll(), then they are able to update the URL

if (queue[0].query) {
const flattened = this.flattenMongoQuery({ query: queue[0].query });
for (const key in flattened) {
Expand Down
Loading