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.

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

yash-builder
Copy link
Contributor

Description:

This PR fixes an issue where meta.componentsUsed is omitted by default when using fetchOneEntry. Previously, to include meta.componentsUsed, you had to explicitly set omit: ' ' (a space) as a workaround.

Changes Made:

  • Updated the logic to include meta.componentsUsed by default unless explicitly omitted.
  • Added omit ?? 'meta.componentsUsed' to ensure componentsUsed is not omitted when omit is set to an empty string ('').

Why This Change Was Made:

To allow meta.componentsUsed to be included without requiring a workaround and to ensure better handling of the omit parameter.

Loom
https://www.loom.com/share/938cd062796c4e2aaaf42b408039713e?sid=f087a66d-478d-42ce-ae41-b8e809535da6

@yash-builder yash-builder added the bug Something isn't working label Mar 18, 2025
@yash-builder yash-builder requested a review from a team March 18, 2025 02:04
@yash-builder yash-builder self-assigned this Mar 18, 2025
@yash-builder yash-builder requested review from samijaber and removed request for a team March 18, 2025 02:04
@yash-builder yash-builder marked this pull request as ready for review March 18, 2025 02:05
Copy link

changeset-bot bot commented Mar 18, 2025

🦋 Changeset detected

Latest commit: 7465cfa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages
Name Type
@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

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

nx-cloud bot commented Mar 18, 2025

View your CI Pipeline Execution ↗ for commit 7465cfa.

Command Status Duration Result
nx test @e2e/gen1-next14-pages ❌ Failed 5m 9s View ↗
nx test @e2e/qwik-city ✅ Succeeded 10m 14s View ↗
nx test @e2e/react-sdk-next-15-app ✅ Succeeded 9m 21s View ↗
nx test @e2e/nextjs-sdk-next-app ✅ Succeeded 8m 55s View ↗
nx test @e2e/angular-16 ✅ Succeeded 8m 25s View ↗
nx test @e2e/nuxt ✅ Succeeded 8m 23s View ↗
nx test @e2e/angular-16-ssr ✅ Succeeded 7m 28s View ↗
nx test @e2e/sveltekit ✅ Succeeded 7m 15s View ↗
Additional runs (36) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-03-18 12:51:22 UTC

@clyde-builderio
Copy link
Contributor

clyde-builderio commented Mar 18, 2025

@yash-builder could you add tests for this?

expect(url).toContain('query.id=29ab534d62c4406c8500e1cbfa609537');

this could be a reference

Copy link
Contributor

@sanyamkamat sanyamkamat left a comment

Choose a reason for hiding this comment

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

  • Do add tests for both Generations SDKs
  • Also add changeset changes

@clyde-builderio clyde-builderio self-requested a review March 18, 2025 04:54
@@ -54,6 +54,18 @@ function App() {
}
}, []);

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the better way would be to add unit test in generate-content-url.test.ts instead of useEffect here. The aim is to respect empty string '' passed to omit field.

@samijaber or @sidmohanty11 can correct me on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The snapshots also should be updated to differentiate omit as undefined vs omit as empty string.
ie generate-content-url.test.ts.snap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants