Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Shallow wrapper #223

Open
wants to merge 67 commits into
base: master
Choose a base branch
from

Conversation

jeparlefrancais
Copy link
Contributor

@jeparlefrancais jeparlefrancais commented Jul 2, 2019

Closes #189 and closes #156.

This PR adds a new feature to Roact: the ability to create snapshots from a Roact tree. Combine to that, the concept of shallow rendering is used to help making as small snapshots as possible.

How it works:

  1. Mounts an element with Roact.mount to get a VirtualTree (as usual)
  2. Get a ShallowWrapper from VirtualTree:getShallowWrapper()
  3. That wrapper is used to make assertions on element kind, props or to find children of a component with some constraints (or simulate events later maybe). It is also possible to generate a snapshot from the ShallowWrapper.

This approach does not have much internal representation, it just uses the VirtualNode to find what it needs. It currently supports fragments.

I have written some documentation about the API and also how snapshots and shallow rendering can be used.

Checklist before submitting:

  • Commit snapshot files
  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

@coveralls
Copy link

coveralls commented Jul 2, 2019

Coverage Status

Coverage increased (+1.3%) to 94.85% when pulling 780fc4f on jeparlefrancais:shallow-wrapper into 986c1d5 on Roblox:master.

@jeparlefrancais
Copy link
Contributor Author

So this just turned into a massive PR, more oriented towards generating snapshots.

The ShallowWrapper will need a better name, but I'm still not sure about what it should be (open for suggestions!)

The current API for snapshots is

local wrapper = Roact.shallow(element, {
    depth = 1,
})

wrapper:toMatchSnapshot("snapshot identifier")

This would generate a snapshot serialized in a ModuleScript in a folder named "RoactSnapshots" under ReplicatedStorage

@jeparlefrancais jeparlefrancais marked this pull request as ready for review July 30, 2019 21:43
return results
end

function ShallowWrapper:findUnique(constraints)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need convenience methods like this? I'm trying to imagine when developers will want to use this versus relying on snapshot matching.

Copy link
Contributor

Choose a reason for hiding this comment

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

We decided to keep this for cases where we want to remove some scaffolding injected by things like mock providers

@MisterUncloaked
Copy link
Contributor

Overall I really like the idea of snapshotting the shallow representation of components versus how snapshots are traditionally done. It keeps the snapshots smaller, and ultimately removes the need for a lot of boilerplate assertions typically found when testing with shallow rendering.

src/VirtualTree.lua Outdated Show resolved Hide resolved
component = element.component,
}
else
error(('shallow wrapper does not support element of kind %q'):format(tostring(kind)))
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder for us to document the types we support when we get to writing documentation for snapshot testing

function ShallowWrapper.new(virtualNode, maxDepth)
virtualNode = findNextVirtualNode(virtualNode, maxDepth)

local wrapper = {
Copy link
Contributor

Choose a reason for hiding this comment

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

to simplify this class, the wrapper could just be the node + the depth, and we could remove some of the mapping we are doing from the virtual node interface to the shallow

end
end

local function filterProps(props)
Copy link
Contributor

Choose a reason for hiding this comment

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

it could be simpler to just not filter the props in the shallow at all

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 want remove the Roact.Children key from the props when it's there. That is because it makes it easier to assert your props. Intuitively, it matches what you put in the props argument of Roact.createElement.

local results = {}

for _, childVirtualNode in pairs(self._virtualNodeChildren) do
getChildren(childVirtualNode, results, self._childrenMaxDepth)
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using a non member function, this could be a bit simpler:

local childWrapper = ShallowWrapper.new(
	childVirtualNode,
	self._childrenMaxDepth
)
childWrapper:getChildren()
table.insert(results, childWrapper)

this would need the fragment check as well if we got rid of the other function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing I don't like with the member function is that I would have to copy back the elements from the getChildren call into the result table recursively. Do you think it would be better to just go for clarity at this point? I have not benchmark anything yet so I don't have numbers about what is the performance gain

Copy link
Contributor

Choose a reason for hiding this comment

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

Perf is a good point, but I wonder if that concern goes away if we redefine depth such that we're building more deeply nested tables instead of coalescing the children at various levels of the tree to be the shallow children.

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Still need to look at the docs a bit more.

I think most of this approach is sound, and really powerful! I am still hopeful, though, that we can find a way to better isolate a lot of this from the main Roact repo.

@@ -32,21 +32,21 @@ Creates a new Roact fragment with the provided table of elements. Fragments allo

### Roact.mount
```
Roact.mount(element, [parent, [key]]) -> RoactTree
Roact.mount(element, [parent, [key]]) -> VirtualTree
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if I agree with changing the terminology here. Ideally, we should expose as little internal vocabulary as possible in order to define what snapshots are and what they do.

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 can switch it back to RoactTree, it's just that the Type we are going to expose in #230 is called VirtualTree. So maybe it could be renamed in that other PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good question! I didn't realize we were exposing it in that PR. We should probably discuss this with @LPGhatguy.

My thoughts are still that VirtualTree probably means less to users, but maybe there's another better alternative too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree that the term VirtualTree might be questionable to users, we should probably solves this in #230 if it's about to be merged first.

docs/api-reference.md Outdated Show resolved Hide resolved
```

If we shallow-render the `MainComponent`, the default depth will not render the CoolComponent.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an explanation of how render depth works, not an explanation of what getHostObject does

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 wanted to explain when getHostObject can return nil or not, but yep it does include talking a bit about depth... Should I remove it or not?


writeSnapshotFile(name, content)
end
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both this and the python version?

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 made both originally because with Roact, lemur runs with lua so I assumed people would prefer lua to sync stuff. And I have made the python version because uiblox is already using python for it's image packaging thing!

So I don't know if we need both, but it's more convenient to offer it I think


---

Using a python interpreter (compatible with 2 or 3), put the following script in a file and run it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we just include the script somewhere?

src/shallow/Snapshot/Serialize/Serializer.lua Show resolved Hide resolved
src/shallow/Snapshot/Serialize/Serializer.lua Outdated Show resolved Hide resolved
src/shallow/Snapshot/Serialize/Serializer.lua Outdated Show resolved Hide resolved
local kind = ElementKind.fromComponent(wrapperComponent)

local typeData = {
kind = kind,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we even still need this in the snapshot, or if there's sufficient data without it.

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 made a longer comment above, but it's useful for function components, because I can't serialize their function. It's only going to show up as a AnonymousFunction symbol

end

return true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This solution seems way overzealous for something like this. Is this in the name of future proofing? I worry it's getting a little out of hand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes at some point I add three parameters (depth, hostKey and hostParent) so I felt like it would be good to make sure there are no typos and mismatched types. But yeah right now with only the depth it looks a bit like nuking a fly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I still keep the dictionary as a parameter for future proofing, but remove the validation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think when it comes to stuff like this, we need to strike a balance between predicting how our API might change in the future and just giving up and making it a table. Tables are fine in a lot of cases but they feel really awkward when there are only one or two elements.

Personally, I'm okay with the public shallow function just taking depth as its second argument. It doesn't seem like we would wildly change this function's signature anytime soon. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and you make me realise that since we are now mounting with Roact.mount, the shallow function is not public anymore. I use it as en entry point from Roact to the shallow module.

@LPGhatguy LPGhatguy added the status: needs design Used when we're not sure what the best way forward is label Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: needs design Used when we're not sure what the best way forward is
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shallow Rendering Snapshot tooling
5 participants