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

Create tests for components defined as refs twice #96

Closed
wants to merge 3 commits into from

Conversation

jspolancor
Copy link
Contributor

With this tests we're trying to solve some questions about defining child components as 'refs' in multiple parents.

@ThaNarie This is what I discovered so far and the questions I have

test if the component is only initialised once: I'm testing the mounting of the child component, it gets mounted once for each parent, is testing the mounting good enough?

test if both refs share the same component instance: I'm unsure how to test this, I could get the global muban instance to test if the child ref exists only once in the instance, does that make sense? the other solution is from the grandparent component look for the parent's child instance like, grandparent.__instance.refs.parent.__instance.refs.child.__instance.uid but feels hacky to me

what happens when using bindings from both places that DON'T overlap:
what happens when using bindings from both places that DO overlap:
what happens with component collections, that maybe partly overlap between parents and children:

The answer for this questions is the same, the bindings are applied from the child like Child > Parent > Grandparent, the grandparent bindings will always overwrite the parent's if they overlap, if they don't overlap then the child updates made by the parent bindings will be preserved, same rule applies to collections

After finishing the tests i still need to write the docs, do you think addding a new section to the component bindings docs is good enough 🍻

@ThaNarie
Copy link
Contributor

test if the component is only initialised once
I'm testing the mounting of the child component, it gets mounted once for each parent, is testing the mounting good enough?

Yes it's good enough for the test, but I don't think this is what we want.
It should only be mounted once, and applied bindings should be set on the same mounted instance.
It'll probably not make a difference in the observed behaviour (which seems to be working mostly okay, luckily), but it does work it shouldn't.

So you could create a separate ticket to change that behaviour.

test if both refs share the same component instance
I'm unsure how to test this...

As you hinted, the only real way to get to internal information is through the __instance. Seems like step 1 is getting access to both Grandparent and Parent, and from there get an instance to the ref for each and compare that (perhaps on different level, the object reference, a uuid, the component instance on the ref, etc).

If we don't have enough/easy internal information to check these kind of things, we might need to invest in things to improve this, like more/easier internal access, and/or utils to work with them, etc.

Let me know if this is enough to get you going, or if I should start looking into it a bit more :)

what happens when using bindings from both places that DON'T overlap:
what happens when using bindings from both places that DO overlap:
what happens with component collections, that maybe partly overlap between parents and children:

The answer for this questions is the same...

Nice, although it surprises me that the parentAttr is null, while the grandparent doesn't override this one. Strangely the isActive does seem to work. Seems like a bug?

Also, would be good to test some compound bindings, like adding different $element bindings, and within that, different attr and event bindings that get merged. So far you've only tested individual component props.

After finishing the tests i still need to write the docs, do you think addding a new section to the component bindings docs is good enough?

I think this is more of a chapter for in the "Guide" area, and then mention and link it from the ignoreGuard option in the API docs.

expect(handleChildClickFromParent).toHaveBeenCalledTimes(1);
// bindings are applied from the child component like Child > Parent > Grandparent
// The final textContent should be the one defined in the grandparent
expect(child.textContent).toBe('text from grandparent');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could make use of the DOM matchers :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a hard time trying to install and configure jest-dom, could you help me with that please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit in your branch to make this work :)

@jspolancor
Copy link
Contributor Author

jspolancor commented Mar 6, 2023

It should only be mounted once, so you could create a separate ticket to change that behaviour.

issue here

If we don't have enough/easy internal information to check these kind of things, we might need to invest in things to improve this, like more/easier internal access, and/or utils to work with them, etc.

I created this issue for ref component instance extraction

Nice, although it surprises me that the parentAttr is null, while the grandparent doesn't override this one. Strangely the isActive does seem to work. Seems like a bug?
I don't think it's a bug, well, it might be, something related to the double mounting, the child component gets remounted and that particular prop is not defined in the grandparent, the prop becomes falsy and the attr gets removed

Also, would be good to test some compound bindings, like adding different $element bindings, and within that, different attr and event bindings that get merged. So far you've only tested individual component props.

A few tests were added

I think this is more of a chapter for in the "Guide" area, and then mention and link it from the ignoreGuard option in the API docs.

I think it's better to finish the two new issues to be completely sure of what's going on before updating the docs

@ThaNarie
Copy link
Contributor

We decided to not allow double refs on a single element, which has been enforced in #99

@ThaNarie ThaNarie closed this Jun 12, 2023
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 this pull request may close these issues.

Test and document what happens when a component element is defined as ref twice
2 participants