fix: do not serialize methods of objects passed to styles

[jakovljevic-mladen]

What is it?

• [ ] Feature / enhancement
• [x] Bug fix
• [ ] Docs / tests / types / typos
• [ ] Infra

Description

It is completely valid to write a code like this:

export const cmp = component$(() => {
  const style = {
    hasOwnProperty: () => false,
    foo: () => {
      return 'bar';
    },
    color: 'red',
  };

  return <a style={style} href="/some-link">Link</a>;
});

This gets serialized to DOM like this:

To prove this wrong, I added some tests for stringifyStyle function that converts whatever it gets from user to a usable string value that later gets stored to DOM. Without this fix, tests "should ignore object methods" and "should stringify object with custom hasOwnProperty method" would fail.

Side note: since I wanted to add a lot of other tests for stringifyStyle (and setValueForStyle as well), I noticed that error with code QError_stringifyClassOrStyle does not mention style property, so I updated error message in packages/qwik/src/core/error/error.ts. Does that make any problem for public facing APIs? If not, then I'd like it to stay because it mentions style as well.

Checklist:

• [x] My code follows the developer guidelines of this project
• [x] I have performed a self-review of my own code
• [x] I have ran pnpm change and documented my changes
• [ ] I have made corresponding changes to the Qwik docs
• [x] Added new tests to cover the fix / functionality

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: e7d826c135646a0aa2b56eed9a399f44ff00bc2b

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

This PR includes changesets to release 4 packages

| Name | Type | | --------------------- | ----- | | @builder.io/qwik | Patch | | eslint-plugin-qwik | Patch | | @builder.io/qwik-city | Patch | | create-qwik | 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

[pkg-pr-new[bot]]

Open in Stackblitz

npm i https://pkg.pr.new/@builder.io/qwik@6932

npm i https://pkg.pr.new/@builder.io/qwik-city@6932

npm i https://pkg.pr.new/eslint-plugin-qwik@6932

npm i https://pkg.pr.new/create-qwik@6932

commit: e7d826c

[github-actions[bot]]

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name	Status	Preview	Last Commit
qwik-docs	✅ Ready (View Log)	Visit Preview	e7d826c135646a0aa2b56eed9a399f44ff00bc2b

[wmertens]

First of all, thank you for your PR!

But, we're about ready to release v2 alpha and we'd need to re-implement this for v2. The code is on the build/v2 branch and serialization all happens in shared-serialization.ts.

Also I am not sure that approach will work there, it will probably strip all functions from objects instead of just for style objects. Normally, qwik should refuse to SSR when it encounters a function but right now we're just ignoring them because we're serializing too much state.

How common is this problem? Is there a workaround?

[jakovljevic-mladen]

@wmertens,

How common is this problem? Is there a workaround?

Not sure how common the problem is, but people can certainly encounter it. I don't think this issue is problematic at all, but this serialization doesn't have to happen, that's all.

Also, I tested this against changes on V2 branch, and this issue is present there as well:

And I went to the v2 branch and I noticed the same util functions exist there as well, so since they're not removed, they're probably still being used in v2 as well. Anyway, since this issue is present in v1, this patch can still be considered for v1.