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

Completely remove the concept of isVirtual #10903

Merged

Conversation

mixonic
Copy link
Sponsor Member

@mixonic mixonic commented Apr 18, 2015

MetamorphView (and the Metamorph mixin) are the final bastion of virtual views. This commit removes much of their use.

MetamorphView had two behaviors: First, it was tagless. Second, it was not included in the view hierarchy. The first concept (taglessness) is still valid in Ember post-Glimmer. For this, we use tagName: ''.

The second concept (virtual views) is not longer present. So views which previously used MetamorphView will not longer be able to escape the view hierarchy. In most cases, the intent was likely to be tagless and being virtual was simply a side effect.

OutletView and EachView will now be present in childViews and will be parentViews.

{{render used to always create a view. Now, it will only create a view if one is set by the user. By default it is only a componentNode and has no view at all.

The final spot MetamorphViews are used it for the default itemView and emptyView on EachView. This should be easy to address in a manner similar to {{render: If the user does not specify a view, none should be required.

After this, MetamorphView will no longer be used by internals.

@stefanpenner
Copy link
Member

isVirtual has been used to prevent intermediate views from appearing in general childViews pool. I know for a fact this will break apps that rely on intermediate views create by each and add-ons like draggable-each not appearing in childViews

Is there an alternative to isVirtualor is the user required to filter out unwanted views when operating on childViews

@rwjblue rwjblue force-pushed the idempotent-rerender-no-virtual branch from 22e3852 to 113e0a7 Compare April 18, 2015 21:06
MetamorphView (and the Metamorph mixin) are the final bastion of
virtual views. This commit removes much of their use.

MetamorphView had two behaviors: First, it was tagless. Second, it was
not included in the view hierarchy. The first concept (taglessness) is
still valid in Ember post-Glimmer. For this, we use `tagName: ''`.

The second concept (virtual views) is not longer present. So views which
previously used MetamorphView will not longer be able to escape the view
hierarchy. In most cases, the intent was likely to be tagless and being
virtual was simply a side effect.

OutletView and EachView will now be present in childViews and will be
parentViews.

`{{render` used to always create a view. Now, it will only create a view
if one is set by the user. By default it is only a componentNode and has
no view at all.

The final spot MetamorphViews are used it for the default itemView and
emptyView on EachView. This should be easy to address in a manner
similar to {{render: If the user does not specify a view, none should be
required.

After this, MetamorphView will no longer be used by internals.
@rwjblue rwjblue force-pushed the idempotent-rerender-no-virtual branch from 113e0a7 to 6368254 Compare April 18, 2015 21:12
@rwjblue
Copy link
Member

rwjblue commented Apr 18, 2015

@stefanpenner - This PR is against the Glimmer branch (not sure if you saw that), and the idea is that there is essentially no such thing as a virtual view any more.

See the commit description in 3aea201 for more details, but here is a snippet:

there are no more virtual views and only concrete views

rwjblue added a commit that referenced this pull request Apr 18, 2015
Completely remove the concept of isVirtual
@rwjblue rwjblue merged commit 44b219c into emberjs:idempotent-rerender Apr 18, 2015
@rwjblue rwjblue deleted the idempotent-rerender-no-virtual branch April 18, 2015 21:27
@stefanpenner
Copy link
Member

and the idea is that there is essentially no such thing as a virtual view any more.

This is clearly going to pwn many app users as the glimmer API lands in 1.x or is this merely part of the opt-in world of glimmerland?

@rwjblue
Copy link
Member

rwjblue commented Apr 19, 2015

@stefanpenner - Why? Are there cases when you actually want to deal with the virtual views?

@stefanpenner
Copy link
Member

@rwjblue i gave the example ^ Lots of code relies on existing virtual views provided by ember and addons, being transparent.

When attrs leaked into childView many things broke, this will do the same.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Apr 19, 2015

The goal, with Glimmer, is that the logical tree of views is the same as the semantic tree. If you do not want a view, don't use one. For example with if does not use a view:

And loc does not use a view:

These will not appear in childViews. This is because there is no view. There is no need for a isVirtual flag to exclude them. I don't believe {{each uses a view, though we continue to expose EachView for semver reasons.

Once the helper API is made public (I believe this is the intent, would appreciate a +1 though) anyone can create view-less logic this way. These helpers do create morphs in a render node tree, and sometimes create a ComponentNode without a view.

Must of how isVirtual worked was already gone before this commit. Virtual views were a clumsy and fragile tool, and I doubt all the side effects and caveats were well understood by most users. They required far more setup and teardown than the current code requires. The small number of use-cases where they were a good solution are mostly or all well handled by the new patterns. They were certainly not something we wanted to make a public API.

Does this assuage your concerns @stefanpenner? Virtual views out, semantic view tree and render nodes in.

@stefanpenner
Copy link
Member

I'm fine with new style code losing this concept but How does affect legacy code relying on its own isVirtual views not polluting the parents childViews list

@mmun
Copy link
Member

mmun commented Apr 19, 2015

@stefanpenner You're right. The isVirtual flag is ignored and it will become part of childNodes.

@stefanpenner
Copy link
Member

You're right. The isVirtual flag is ignored and it will become part of childNodes.

This seems like a breaking change to much code. Even just the attrNodes entering childViews caused pain. I am unsure if we can responsibly introduce this in the 1x timeframe.

If we must, then we should likely doit with a deprecation phase.

@mmun
Copy link
Member

mmun commented Apr 19, 2015

To be clear, draggable-each will break for other reasons (it is using _childViews which was removed). Anyways, let's focus on isVirtual. It sounds like you're saying that you have seen code in the wild that relies on the fact that draggable-each is virtual, and so that it's child views get lifted into a parent view's children.

This seems mega crazy to me. The only time I can see this mattering is if you have a CollectionView subclass whose itemViewClass is a DraggableEach AND you were expecting the collection view's childViews array to be a flattened, concat of all of the draggable-each's together.

Can you explain further?

@stefanpenner
Copy link
Member

This seems mega crazy to me.

but this was the actual intent of isVirtual.

Although draggableEach will break but I have seen several other projects (app code) that rely on isVirtual in this same way. We can fix draggableEach, we can't fix other peoples app code.

ChildViews being contaminated with once virtual views will be an upgrade hazard, and as such should go through a deprecation phase. Luckily 1.12 could contain this deprecation phase, before the above code theoretically lands in 1.13.

If a deprecation phase exists, I suspect we have done our due diligence.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Apr 20, 2015

After some discussion, we've decided that deprecating use of isVirtual and Ember._MetamorphView by external consumers in 1.12 is a prudent move. In 1.13 those two features will be removed.

I believe @rwjblue (the legend) claimed he would pick this up.

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.

None yet

4 participants