-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix: [ENG-8644] overriding omit in fetchOneEntry to be empty string #3975
Changes from all commits
24b8a82
ae556d4
147894b
a2ba43d
1d5ffaa
fa39349
7465cfa
49c6374
ef692ab
8757b63
21ff5f3
a2b95c1
0f3adea
5269cef
7253cd0
de94ff8
983f8bb
fe7154d
ed6ab0b
8008e53
a75c8fe
6e1b15c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
--- | ||
"@builder.io/sdk": patch | ||
"@builder.io/react": patch | ||
"@builder.io/sdk-angular": patch | ||
"@builder.io/sdk-react-nextjs": patch | ||
"@builder.io/sdk-qwik": patch | ||
"@builder.io/sdk-react": patch | ||
"@builder.io/sdk-react-native": patch | ||
"@builder.io/sdk-solid": patch | ||
"@builder.io/sdk-svelte": patch | ||
"@builder.io/sdk-vue": patch | ||
--- | ||
|
||
Fix: correctly set default value for `omit` field as `meta.componentsUsed` in Content API calls and preserve empty string |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,8 +41,9 @@ test.describe('Get Content', () => { | |
expect(await req!.postDataJSON()).toEqual({ test: 'test' }); | ||
expect(req!.method()).toBe('POST'); | ||
}); | ||
test('fetch symbol with query.id', async ({ page, sdk }) => { | ||
test('fetch symbol with query.id', async ({ page, sdk, packageName }) => { | ||
test.skip(!excludeGen1(sdk)); | ||
test.skip(packageName !== 'gen1-next14-pages'); | ||
|
||
let x = 0; | ||
let headers; | ||
|
@@ -78,4 +79,94 @@ test.describe('Get Content', () => { | |
expect(headers?.['x-builder-sdk-gen']).toBe(getSdkGeneration(sdk)); | ||
expect(headers?.['x-builder-sdk-version']).toMatch(/\d+\.\d+\.\d+/); // Check for semver format | ||
}); | ||
|
||
test('should NOT omit componentsUsed when omit parameter is explicitly set to empty string', async ({ | ||
page, | ||
sdk, | ||
}) => { | ||
test.skip(!excludeGen1(sdk)); | ||
yash-builder marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let builderRequestPromise: Promise<string> | undefined = undefined; | ||
let requestUrl: string | undefined; | ||
|
||
const builderApiRegex = /https:\/\/cdn\.builder\.io\/api\/v3\//; | ||
|
||
await page.goto('/get-content-with-omit'); | ||
|
||
builderRequestPromise = new Promise<string>(resolve => { | ||
page.on('request', request => { | ||
const url = request.url(); | ||
if (builderApiRegex.test(url)) { | ||
requestUrl = url; | ||
resolve(url); | ||
} | ||
}); | ||
}); | ||
|
||
await page.reload({ waitUntil: 'networkidle' }); | ||
await builderRequestPromise; | ||
|
||
expect(requestUrl).toBeDefined(); | ||
expect(requestUrl!).not.toContain('omit=meta.componentsUsed'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. confused by this test: title says There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh my bad....the test should revolve around both the cases where it should be present and not be present based o the given scenario There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah ok, could you update it to test both 🙏🏽 |
||
expect(requestUrl!.includes('omit=')).toBeTruthy(); | ||
expect(new URL(requestUrl!).searchParams.get('omit')).toBe(''); | ||
}); | ||
|
||
test('should omit the specified field when omit parameter has a defined value for gen1', async ({ | ||
page, | ||
sdk, | ||
}) => { | ||
test.skip(!excludeGen1(sdk)); | ||
|
||
let builderRequestPromise: Promise<string> | undefined = undefined; | ||
let requestUrl: string | undefined; | ||
|
||
const builderApiRegex = /https:\/\/cdn\.builder\.io\/api\/v3\//; | ||
|
||
await page.goto('/get-content-with-omit-name'); | ||
|
||
builderRequestPromise = new Promise<string>(resolve => { | ||
page.on('request', request => { | ||
const url = request.url(); | ||
if (builderApiRegex.test(url)) { | ||
requestUrl = url; | ||
resolve(url); | ||
} | ||
}); | ||
}); | ||
|
||
await page.reload({ waitUntil: 'networkidle' }); | ||
await builderRequestPromise; | ||
|
||
expect(requestUrl).toBeDefined(); | ||
expect(requestUrl!).toContain('omit=name'); | ||
expect(new URL(requestUrl!).searchParams.get('omit')).toBe('name'); | ||
}); | ||
|
||
test('should use default omit value when omit parameter is undefined', async ({ page, sdk }) => { | ||
test.skip(!excludeGen1(sdk)); | ||
|
||
let builderRequestPromise: Promise<string> | undefined = undefined; | ||
let requestUrl: string | undefined; | ||
|
||
const builderApiRegex = /https:\/\/cdn\.builder\.io\/api\/v3\//; | ||
|
||
await page.goto('/get-content-default'); | ||
|
||
builderRequestPromise = new Promise<string>(resolve => { | ||
page.on('request', request => { | ||
const url = request.url(); | ||
if (builderApiRegex.test(url)) { | ||
requestUrl = url; | ||
resolve(url); | ||
} | ||
}); | ||
}); | ||
|
||
await page.reload({ waitUntil: 'networkidle' }); | ||
await builderRequestPromise; | ||
|
||
const omitValue = new URL(requestUrl!).searchParams.get('omit'); | ||
expect(omitValue).toBe('meta.componentsUsed'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,21 +28,6 @@ exports[`Component Registration and Serialization > serializeComponentInfo handl | |
} | ||
`; | ||
|
||
exports[`Component Registration and Serialization > serializeComponentInfo handles async arrow functions correctly 1`] = ` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh didn't notice that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm....It seems this was added twice that's the reason it is being removed |
||
{ | ||
"inputs": [ | ||
{ | ||
"name": "testInput", | ||
"onChange": "return (async (value) => { | ||
return value.toUpperCase(); | ||
}).apply(this, arguments)", | ||
"type": "string", | ||
}, | ||
], | ||
"name": "TestComponent", | ||
} | ||
`; | ||
|
||
exports[`Component Registration and Serialization > serializeComponentInfo handles async arrow functions without parenthesis correctly 1`] = ` | ||
{ | ||
"inputs": [ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why this extra skip was added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so only
gen1-next14-pages
is the one where API's are not getting called. That's the reason I added thisskip
. Please do let me know if otherwiseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused since the test has been running all this time without being skipped for Next14 Pages and passing.
Ah. Not in this case. This test is checking content that has a symbol, BUT where the symbol's full content was not downloaded in the initial JSON:
builder/packages/sdks-tests/src/specs/index.ts
Line 237 in 4b78d6c
builder/packages/sdks-tests/src/specs/symbols.ts
Lines 732 to 737 in 4b78d6c
So in this scenario, we're testing that the Symbol block correctly fetches its own content when it is not already present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samijaber
Could you clarify what you mean by the Symbol block fetching its own content when it's not already present? I’m a bit unclear on that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when data returned from Content API through
fetchOneEntry
includes a symbol, there are two ways it can play out:Symbol
can render immediately<Symbol>
component ends up fetching its own content. Seebuilder/packages/sdks/src/blocks/symbol/symbol.lite.tsx
Lines 73 to 84 in e1af39b
The test where you added
test.skip(packageName !== 'gen1-next14-pages');
is the second case: the Content JSON is missing the Symbol reference, so we are testing that the API calls tohttps://cdn.builder.io/api/v3/content/symbol
are made correctly.Essentially, I don't believe ^this statement is accurate: those API calls should be made in every gen1 test server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right—but after extensive debugging, I found that the API call doesn't happen the first time; it only triggers after a reload. Let me clarify this further.
Attaching loom for the reference: Loom