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

Vue SDK: stop passing down unused props #3891

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

Vue SDK: stop passing down unused props #3891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b71c302
f test
samijaber Feb 4, 2025
a576fe4
👹
samijaber Feb 4, 2025
e6bf304
🏃
samijaber Feb 4, 2025
c2e9a5c
🏃
samijaber Feb 4, 2025
e10912b
🏃
samijaber Feb 4, 2025
6cc4cdd
Merge branch 'main' of github.com:BuilderIO/builder into fix/vue-sdk-…
samijaber Feb 5, 2025
157e72f
tmp
samijaber Feb 5, 2025
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
32 changes: 26 additions & 6 deletions packages/sdks-tests/src/helpers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ async function screenshotOnFailure(
}
}

const isIgnorableError = (error: Error) => {
return error.message.includes(
/**
* This error started appearing recently across all frameworks.
* It is most likely some playwright browser issue and not something we can fix in our code.
*/
"Failed to execute 'observe' on 'PressureObserver': Access to the feature \"compute pressure\" is disallowed by permissions policy"
);
};

const test = base.extend<TestOptions>({
// this is provided by `playwright.config.ts`
packageName: ['DEFAULT' as any, { option: true }],
Expand All @@ -45,19 +55,23 @@ const test = base.extend<TestOptions>({
ignoreHydrationErrors: [false, { option: true }],
page: async ({ context, page, packageName, sdk, ignoreHydrationErrors }, use) => {
if (packageName === ('DEFAULT' as any)) {
throw new Error('packageName is required');
throw new Error('`packageName` is required but was not provided.');
}
if (sdk === ('DEFAULT' as any)) {
throw new Error('sdk is required');
throw new Error('`sdk` is required but was not provided.');
}

context.on('weberror', err => {
if (isIgnorableError(err.error())) return;

console.error(err.error());
throw new Error('Failing test due to error in browser: ' + err.error());
throw new Error('Test failed due to error thrown in browser: ' + err.error());
});
page.on('pageerror', err => {
if (isIgnorableError(err)) return;

console.error(err);
throw new Error('Failing test due to error in browser: ' + err);
throw new Error('Test failed due to error thrown in browser: ' + err);
});

/**
Expand All @@ -80,16 +94,21 @@ const test = base.extend<TestOptions>({
}
});
}

if (sdk === 'angular') {
page.on('console', msg => {
const originalText = msg.text();
if (originalText.includes('NG0303')) {
throw new Error('Angular input not annotated error detected: ' + originalText);
}
});
} else if (sdk === 'vue') {
page.on('console', msg => {
const originalText = msg.text();
if (originalText.toLowerCase().includes('[vue warn]:')) {
throw new Error('Vue warning detected: ' + originalText);
}
});
}

await use(page);
},
});
Expand Down Expand Up @@ -219,6 +238,7 @@ export const checkIfIsHydrationErrorMessage = (_text: string) => {
const text = _text.toLowerCase();
const isVueHydrationMismatch =
text.includes('[vue warn]') && (text.includes('hydration') || text.includes('mismatch'));

const isReactHydrationMismatch =
text.includes('did not expect server') ||
text.includes('content does not match') ||
Expand Down
4 changes: 2 additions & 2 deletions packages/sdks/mitosis.config.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,8 +773,8 @@ const ANGULAR_NOWRAP_INTERACTIVE_ELEMENT_PLUGIN = () => ({
`
);
code = code.replaceAll(
'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:

builder/packages/sdks/mitosis.config.cjs

Lines 723 to 731 in f9b8552

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

);

// extract the props that Wrapper needs
Expand Down
3 changes: 1 addition & 2 deletions packages/sdks/overrides/svelte/src/components/awaiter.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
export let load: () => Promise<{ default: typeof SvelteComponent<any> }>;
export let fallback: any;
export let props: any;
export let attributes: any;

const componentImport = typeof load === 'string' ? import(load) : load();
</script>
Expand All @@ -14,7 +13,7 @@
<svelte:component this={fallback} />
{/if}
{:then { default: Component }}
<svelte:component this={Component} {...props} {attributes}>
<svelte:component this={Component} {...props}>
<slot />
</svelte:component>
{/await}
11 changes: 7 additions & 4 deletions packages/sdks/src/blocks/text/component-info.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ import { TARGET } from '../../constants/target.js';
import type { ComponentInfo } from '../../types/components.js';

export const componentInfo: ComponentInfo = {
shouldReceiveBuilderProps: {
builderBlock: TARGET === 'reactNative' ? true : false,
builderContext: true,
},
shouldReceiveBuilderProps:
TARGET === 'reactNative'
? {
builderBlock: true,
builderContext: true,
}
: {},
name: 'Text',
static: true,
isRSC: true,
Expand Down
2 changes: 0 additions & 2 deletions packages/sdks/src/components/awaiter.lite.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { onMount, useTarget } from '@builder.io/mitosis';
type AwaiterProps = {
load: () => Promise<any>;
props?: any;
attributes?: any;
fallback?: any;
children?: any;
};
Expand All @@ -21,7 +20,6 @@ export default function Awaiter(props: AwaiterProps) {
const _ = {
a: props.load,
b: props.props,
c: props.attributes,
d: props.fallback,
};
},
Expand Down
14 changes: 9 additions & 5 deletions packages/sdks/src/components/block/components/interactive-element.lite.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ useMetadata({
*/
export default function InteractiveElement(props: InteractiveElementProps) {
const state = useStore({
get attributes() {
return props.includeBlockProps
get wrappedPropsWithAttributes() {
const attributes = props.includeBlockProps
? {
...getBlockProperties({
block: props.block,
Expand All @@ -48,23 +48,27 @@ export default function InteractiveElement(props: InteractiveElementProps) {
}),
}
: {};

return {
...props.wrapperProps,
...(attributes ? { attributes } : {}),
};
},
});

return (
<Show
when={props.Wrapper.load}
else={
<props.Wrapper {...props.wrapperProps} attributes={state.attributes}>
<props.Wrapper {...state.wrappedPropsWithAttributes}>
{props.children}
</props.Wrapper>
}
>
<Awaiter
load={props.Wrapper.load}
fallback={props.Wrapper.fallback}
props={props.wrapperProps}
attributes={state.attributes}
props={state.wrappedPropsWithAttributes}
>
{props.children}
</Awaiter>
Expand Down
Loading