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

Investigation, delete value.value to access some components #160

Closed
4 tasks done
JaimeTorrealba opened this issue Aug 2, 2023 · 9 comments
Closed
4 tasks done

Investigation, delete value.value to access some components #160

JaimeTorrealba opened this issue Aug 2, 2023 · 9 comments
Labels
help wanted Extra attention is needed investigation

Comments

@JaimeTorrealba
Copy link
Member

Description

Currently some abstraction in cientos, need to be access by value.value at the moment of reference them

As a good DX we need to find a solution to this detail.

The problem is located in the EJ:

defineExpose({
value: starsRef,
})

Suggested solution

If you have a good solution, please explain it here

Alternative

No response

Additional context

No response

Validations

@JaimeTorrealba JaimeTorrealba added help wanted Extra attention is needed investigation labels Aug 2, 2023
@JaimeTorrealba JaimeTorrealba changed the title Investigation, delete value.value in some abs Investigation, delete value.value to access some components Aug 11, 2023
@JaimeTorrealba
Copy link
Member Author

@andretchen0 bro I trust a lot in you, so sorry if you feel I'm given you work (please don't think so, I would like your comments in this subject)

As you know soon will be release the v4 and is our opportunity to introduce some breaking change to improve the pkg.

How do you feel about this? Are you comfortable with the value.value pattern? Do you have any problems with it?
I personally don't like it, but my solutions also don't convince me 😮‍💨

I don't feel good that if you want to access to (for example a box/shape) you have to do it with

const boxRef = shallowRef()

watch(boxRef , value =>{
value.value = .....
})

// or worse

onLoop(() => {
 if (boxRef.value){
 boxRef.value.value.position.x = ...
 }
})

@JaimeTorrealba
Copy link
Member Author

@alvarosabu Your appreciation here will be highly valuable too.

As far I can see, we could standardize this one with other word, like "current" or "instance". Having another point of view will be fantastic.

Or we could just leave as it is.

Of course no rush

@alvarosabu
Copy link
Member

I completely agree @JaimeTorrealba , what about ref since is a reference of the instance, or well instance is ok to, whatever is shorter for the chaining boxRef.ref.value

We would need to replace:

defineExpose({
  value: controlsRef,
})

With

defineExpose({
  ref: controlsRef,
})

@JaimeTorrealba
Copy link
Member Author

JaimeTorrealba commented Feb 8, 2024

In that case will be boxRef.value.ref, but yeah I like it

@andretchen0
Copy link
Contributor

andretchen0 commented Feb 9, 2024

Hey @JaimeTorrealba @alvarosabu ,

How do you feel about this? Are you comfortable with the value.value pattern? Do you have any problems with it? I personally don't like it, but my solutions also don't convince me 😮‍💨

I agree. I would like to change value.value.

Idea 1

For Cientos, we're mostly just exposing the root Object3D. What do you guys think of this?

<script setup>
// ...
// NOTE: the Object3D that will serve as the root for this component
const root = new Object3D();
defineExpose(root)
</script>

In the parent, it's used like this:

<script setup>
// ...
const levioso = ref(null)
const box = ref(null)
watch(levioso, () => console.log(levioso.value.isObject3D))
watch(box, () => console.log(box.value.isObject3D))
</script>

<template>
<Levioso ref="levioso">
    <Box ref="box" />
</Levioso>
</template>

Idea 2

Otherwise, if we want to leave room for exposing other fields, maybe ... :

defineExpose({
    root,
    someUsefulFunction
})

... which gives us ... :

watch(levioso, () => console.log(levioso.value.root.isObject3D))

I think many/most Vue/Three users will understand "root" (as in "root node of the child") without need for further explanation. And "root" couples nicely with digging down into the subtree:

levioso.value.root.children

We'd need to figure out other names for e.g., what's in defineExpose from useFBO.

@alvarosabu
Copy link
Member

Hi, @andretchen0 I remember we added the .value to the expose because you can't pass ref or reactive, only plain objects. dunno if it's still a restriction.

I like the second idea.

@andretchen0
Copy link
Contributor

I remember we added the .value to the expose because you can't pass ref or reactive, only plain objects. dunno if it's still a restriction.

Sure thing. As of today, passing plain values or refs is documented.

@alvarosabu
Copy link
Member

Let's do it. @JaimeTorrealba please create a todo list of all the components that need change on the ticket and let's go for it.

Since this will be a breaking change, please open the branch from v4

@JaimeTorrealba
Copy link
Member Author

Perfect. Let's stick with "root" as Andre points, we could expose more than one instance (method, etc).

Wait a PR soon ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed investigation
Projects
Status: Done
Development

No branches or pull requests

3 participants