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

/* @once */ seems to work only for first child of object #252

Open
titoBouzout opened this issue May 20, 2023 · 7 comments
Open

/* @once */ seems to work only for first child of object #252

titoBouzout opened this issue May 20, 2023 · 7 comments

Comments

@titoBouzout
Copy link
Contributor

Say we have

function Comp(props) {
  return (
    <div style={/* @once */ { width: props.width, height: props.height }} />
  );
}

generated code is

function Comp(props) {
  return (() => {
    const _el$ = _tmpl$();

    props.width != null ? 
        _el$.style.setProperty("width", props.width) :
         _el$.style.removeProperty("width");

    _$effect(() => props.height != null ? 
        _el$.style.setProperty("height", props.height) :
         _el$.style.removeProperty("height"));

    return _el$;
  })();
}

Playground: https://playground.solidjs.com/anonymous/03735897-b261-4420-835c-7e69c27ad978

@ryansolid
Copy link
Owner

Yeah style and classList are special in that they analyze on a per object expression level. So in this case put the once in front of each property.

<div style={ { width: /* @once */props.width, height: /* @once */props.height }} />

I realize there is nothing stopping us from oncing the whole object as well.

@edemaine
Copy link
Contributor

edemaine commented Jun 1, 2023

The current behavior is particularly surprising when style is set to something other than an object literal:

<div style={/* @once */ props.style} />

Currently /* @once */ is ignored in this case. Playground

In any case, I've added some documentation about the current behavior so this is less surprising.

@titoBouzout
Copy link
Contributor Author

I was just looking how to fix this, the logic seems to be here https://github.com/ryansolid/dom-expressions/blob/main/packages/babel-plugin-jsx-dom-expressions/src/shared/utils.js#L87-L94
Im staring at this not knowing what to do with that information

@titoBouzout
Copy link
Contributor Author

#258

@ryansolid
Copy link
Owner

Yeah I have to admit when I added these features they were compiler only originally. There has been talk about just deprecating @once all together. It's nice to have the escape hatch. But it is also so specific. And actually doesn't guarantee once, just removes the specific tracking scope.

@titoBouzout
Copy link
Contributor Author

titoBouzout commented Jun 7, 2023

I heard the expectation is to use a memo instead, is there other approach? Looks to me a bit of a waste, as there's going to be an effect created that is not going to be used, code would be bigger.

For the case in place, can you consider merging it regardless of the future of this feature, it makes it work closer to the intention and of what's expected.

@titoBouzout
Copy link
Contributor Author

see also solidjs/solid#1938

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

No branches or pull requests

3 participants