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

Vue SDK: stop passing down unused props #3891

Merged
merged 19 commits into from
Mar 27, 2025

Conversation

samijaber
Copy link
Contributor

Copy link

changeset-bot bot commented Feb 4, 2025

🦋 Changeset detected

Latest commit: b333015

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

This PR includes changesets to release 7 packages
Name Type
@builder.io/sdk-vue 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-solid Patch
@builder.io/sdk-svelte 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 Feb 4, 2025

View your CI Pipeline Execution ↗ for commit b333015.

Command Status Duration Result
nx test @e2e/qwik-city ❌ Failed 8m 20s View ↗
nx test @e2e/nuxt ❌ Failed 6m 45s View ↗
nx test @e2e/svelte ✅ Succeeded 10m 32s View ↗
nx test @e2e/solid-start ✅ Succeeded 10m 44s View ↗
nx test @e2e/gen1-next14-pages ✅ Succeeded 9m 27s View ↗
nx test @e2e/nextjs-sdk-next-app ✅ Succeeded 9m 1s View ↗
nx test @e2e/react-sdk-next-pages ✅ Succeeded 7m 9s View ↗
nx test @e2e/gen1-react ✅ Succeeded 8m 10s View ↗
Additional runs (36) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-03-27 12:34:47 UTC

'attributes: this.attributes,',
'...(this.hasAttributesInput(this.Wrapper) ? { attributes: this.attributes } : {})'
'attributes: this.wrappedPropsWithAttributes.attributes,',
'...(this.hasAttributesInput(this.Wrapper) ? { attributes: this.wrappedPropsWithAttributes.attributes } : {})'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sidmohanty11 I think I'm breaking this code with this PR. My change here is fine but the ngOnChanges overrides below in this same plugin will stop working:

ngOnChanges(changes: SimpleChanges) { if (changes["attributes"] && !this.hasAttributesInput(this.Wrapper)) { this.ngAfterViewInit(); }

what do you suggest we do about this? Also, is there anything else that will start to break with this approach?

if it's too hard to make the angular SDK happy with not having an attributes prop, i can look into bringing it back in this plugin instead

Copy link
Contributor

Choose a reason for hiding this comment

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

@samijaber breaking up attributes will mean that we'll lose access to builder-id, style etc so now we'll have to attach all props as HTML attributes and not pass them to be reactive (as inputs). Is it possible to keep the attributes key? because here:

ngAfterViewInit() {
if (!this.hasAttributesInput(this.Wrapper)) {
const wrapperElement =
this.wrapperTemplateRef.elementRef.nativeElement?.nextElementSibling;
if (wrapperElement) {
this.updateAttributes(wrapperElement, this.attributes);
}
}
}

we check if that element doesn't have attributes as an input then we spread it out in the element

Copy link
Contributor

Choose a reason for hiding this comment

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

else we can look for only these four: id, style, href and className as attributes and send everything else as inputs

@samijaber samijaber force-pushed the fix/vue-sdk-extra-props branch from 157e72f to 4740de2 Compare March 25, 2025 18:21
Copy link

gitguardian bot commented Mar 25, 2025

⚠️ GitGuardian has uncovered 5 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9071768 Triggered Generic High Entropy Secret 4740de2 packages/sdks-tests/src/specs/video-lazy-load.ts View secret
2708648 Triggered Generic High Entropy Secret 4740de2 examples/angular-gen2/src/environments/environment.ts View secret
9071769 Triggered Generic High Entropy Secret 4740de2 packages/sdks-tests/src/specs/video-lazy-load.ts View secret
314150 Triggered Generic High Entropy Secret 4740de2 packages/sdks/src/functions/get-content/snapshots/generate-content-url.test.ts.snap View secret
11707119 Triggered Generic High Entropy Secret 4740de2 packages/sdks/snippets/sveltekit/src/routes/editable-region/+page.svelte View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@samijaber samijaber removed the request for review from sidmohanty11 March 26, 2025 17:09
@samijaber samijaber enabled auto-merge (squash) March 26, 2025 17:09
Copy link
Contributor

@sidmohanty11 sidmohanty11 left a comment

Choose a reason for hiding this comment

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

@samijaber this needs a changeset

as the accordion VE is actually failing: https://www.loom.com/share/036fda4959c9473aa0604299097b7a64 we can skip it for now

@samijaber
Copy link
Contributor Author

@samijaber this needs a changeset

The student has become the master

@samijaber samijaber disabled auto-merge March 27, 2025 12:21
@samijaber samijaber enabled auto-merge (squash) March 27, 2025 12:22
@samijaber samijaber requested a review from sidmohanty11 March 27, 2025 12:22
Copy link
Contributor

@sidmohanty11 sidmohanty11 left a comment

Choose a reason for hiding this comment

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

🚀

@samijaber samijaber disabled auto-merge March 27, 2025 15:12
@samijaber samijaber merged commit e12cff4 into BuilderIO:main Mar 27, 2025
44 of 46 checks passed
@samijaber samijaber deleted the fix/vue-sdk-extra-props branch March 27, 2025 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants