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

<div>{ new String('hi') }</div> renders blank #4151

Closed
1 task done
appsforartists opened this issue Oct 4, 2023 · 4 comments · Fixed by #4152
Closed
1 task done

<div>{ new String('hi') }</div> renders blank #4151

appsforartists opened this issue Oct 4, 2023 · 4 comments · Fixed by #4152

Comments

@appsforartists
Copy link
Contributor

  • Check if updating to the latest Preact version resolves the issue

Describe the bug

render(
  <div>
    { new String('this should work') }
  </div>,
  root
);

doesn't render the string. This works in React.

To Reproduce

React and Preact are both shown here

Note

Normally I would just render a string primitive; however, some of the strings I want to render may have metadata attached to them (not relevant to the render, but useful in the app).

const thing = 'thing';
thing.metadata = 123;

is illegal; however,

const thing = new String('thing');
thing.metadata = 123;

works. Thus, I'm trying to figure out how I can have an object that is treated like a string e.g. by Preact, but can have metadata attached when needed.

@appsforartists appsforartists changed the title <div>{ new String('hi')</div> renders blank <div>{ new String('hi') }</div> renders blank Oct 4, 2023
@JoviDeCroock
Copy link
Member

I think you would have to call .toString() on your new String() constructed object as essentially this is an object in the VDom which will run into a security measure of Preact to block injected nodes https://github.com/preactjs/preact/blob/main/src/diff/index.js#L41-L43

@appsforartists
Copy link
Contributor Author

Can you please explain that attack?

As I showed in the Colab, React supports the String constructor. Do they mitigate against it?

Seems like you could do something like change:

if (newVNode.constructor !== undefined) return null;

to

if (newVNode.toString) // or newVNode.constructor === String
  newVNode = newVNode.toString();
else if (newVNode.constructor !== undefined) return null;

@appsforartists
Copy link
Contributor Author

That's not it.

`abc`.constructor === String

@appsforartists
Copy link
Contributor Author

I updated the Colab to include an example after #4152 is applied.

andrewiggins added a commit that referenced this issue Nov 8, 2023
In `constructNewChildrenArray` we look for the `String` constructor to support #4151 (i.e. strings created with `new String`). However, doing this check first leads to a megamorphic deopt in `constructorNewChildrenArray` since V8 has many different internal types for strings, leading to a megamorphic access for `childVNode.constructor` in the common case (normal strings and VNodes).

This PR adds back the `typeof childVNode == 'string'` check to catch normal strings first before falling back to `childVNode.constructor == String` to check for manually constructed strings.

Commits:
* Add back typeof string check in diffChildren (+5 B)
* Store matching oldVNode in variable in constructNewChildrenArray  (-4 B)
* Inline insertion checks into skew block (-3 B)
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 a pull request may close this issue.

2 participants