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

Unmount DOM when `if` becomes false #1658

Merged
merged 10 commits into from Apr 13, 2016

Conversation

Projects
None yet
8 participants
@rogueg
Member

rogueg commented Mar 7, 2016

This is a branch I've been hacking on for a couple weeks. I'm not 100% sure it's ready for merging, but I'd really like to get some feedback, and maybe some help testing/sanity-checking it.

The basic idea is that when an if statement becomes false, we want to remove that DOM, unmount it, and throw it away. The means refactoring how unmounting work, and also adds the notion that expressions (like the id and name) attributes can be unmounted.

There are a handful of breaking changes here:

  • When a tag is removed and then re-added by an if, it's state will now be completely reset. This is true of custom tags, as well as things like inputs.
  • id and name will no longer set a ref on the parent tag if they're within a false if.
  • The Tag.tags collection won't contain child tags if they're within a false if.
  • Tag.tags is no longer affected by name, which I found really unpredictable.
  • Both refs and Tag.tags will no longer match the order of elements in DOM.
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Mar 8, 2016

Member

This looks an amazing improvement for riot. I will check your code and merge asap. In the meantime thank you so much for your contribution. I am thinking whether it makes sense to let you join the riot group. Of course if you agree and the others have nothing against it.
cc @aMarCruz @rsbondi @tipiirai @cognitom

Member

GianlucaGuarini commented Mar 8, 2016

This looks an amazing improvement for riot. I will check your code and merge asap. In the meantime thank you so much for your contribution. I am thinking whether it makes sense to let you join the riot group. Of course if you agree and the others have nothing against it.
cc @aMarCruz @rsbondi @tipiirai @cognitom

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Mar 8, 2016

Member

join 👍

Member

aMarCruz commented Mar 8, 2016

join 👍

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Mar 8, 2016

Member

Would be a welcome addition

Member

rsbondi commented Mar 8, 2016

Would be a welcome addition

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Mar 14, 2016

Member

@rogueg do you want to join us?

Member

GianlucaGuarini commented Mar 14, 2016

@rogueg do you want to join us?

@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Mar 14, 2016

Member

I think riot is a great library, and I'd love to join. Thanks!

Member

rogueg commented Mar 14, 2016

I think riot is a great library, and I'd love to join. Thanks!

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Mar 14, 2016

Member

@rogueg I ve sent you an invitation to join the riot organization. You will be able to work on the next branch without being forced always to make pull requests to update the code. I will check this pull request asap and I hope to be able to build riot with rollup asap

Member

GianlucaGuarini commented Mar 14, 2016

@rogueg I ve sent you an invitation to join the riot organization. You will be able to work on the next branch without being forced always to make pull requests to update the code. I will check this pull request asap and I hope to be able to build riot with rollup asap

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Mar 14, 2016

Member

Welcome to the team @rogueg. Do you want to take a look at #1659 ? I think you have the solution already with #1525 but I don't think it ever made it into the code base.

Member

rsbondi commented Mar 14, 2016

Welcome to the team @rogueg. Do you want to take a look at #1659 ? I think you have the solution already with #1525 but I don't think it ever made it into the code base.

@rogueg rogueg referenced this pull request Mar 15, 2016

Closed

Virtual elements disappear after tag update #1659

1 of 1 task complete
@tipiirai

This comment has been minimized.

Show comment
Hide comment
@tipiirai

tipiirai Mar 15, 2016

Member

Welcome @rogueg !

Member

tipiirai commented Mar 15, 2016

Welcome @rogueg !

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Mar 16, 2016

Member

Super cool! Welcome @rogueg !

Member

cognitom commented Mar 16, 2016

Super cool! Welcome @rogueg !

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Mar 18, 2016

Member

@rogueg welcome to the riot team!

Member

aMarCruz commented Mar 18, 2016

@rogueg welcome to the riot team!

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Mar 19, 2016

Member

I was checking this pull request but before merging it into the next branch can you guys (either @rsbondi or @rogueg ) sync it with the dev branch please?

Member

GianlucaGuarini commented Mar 19, 2016

I was checking this pull request but before merging it into the next branch can you guys (either @rsbondi or @rogueg ) sync it with the dev branch please?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Mar 22, 2016

Member

@rogueg can you send me your email address please?

Member

GianlucaGuarini commented Mar 22, 2016

@rogueg can you send me your email address please?

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Mar 22, 2016

Member

I will try the merge of the dev branch changes this weekend unless @rogueg insists on doing it.

Member

rsbondi commented Mar 22, 2016

I will try the merge of the dev branch changes this weekend unless @rogueg insists on doing it.

@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Mar 24, 2016

Member

I'm actually moving this week, so not a ton of free time. Thanks for taking a stab at merging it @rsbondi! My email is rogueg@gmail.com

Member

rogueg commented Mar 24, 2016

I'm actually moving this week, so not a ton of free time. Thanks for taking a stab at merging it @rsbondi! My email is rogueg@gmail.com

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Mar 26, 2016

Member

I will not change or add new code into the dev branch waiting your merge @rsbondi so we avoid too many merge conflicts.. In the meantime I will release riot 2.3.18

Member

GianlucaGuarini commented Mar 26, 2016

I will not change or add new code into the dev branch waiting your merge @rsbondi so we avoid too many merge conflicts.. In the meantime I will release riot 2.3.18

@GianlucaGuarini GianlucaGuarini referenced this pull request Apr 7, 2016

Closed

Programatic alternative to the "if" tag property #1725

1 of 7 tasks complete
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 8, 2016

Member

Now the dev and the next branch are in sync. @rogueg could you please update the pull request? I will be happy to merge it

Member

GianlucaGuarini commented Apr 8, 2016

Now the dev and the next branch are in sync. @rogueg could you please update the pull request? I will be happy to merge it

@john-goldsmith

This comment has been minimized.

Show comment
Hide comment
@john-goldsmith

john-goldsmith Apr 12, 2016

@rogueg Can you please update your PR so this can get merged? Thanks! 🍒

john-goldsmith commented Apr 12, 2016

@rogueg Can you please update your PR so this can get merged? Thanks! 🍒

@rma4ok

This comment has been minimized.

Show comment
Hide comment
@rma4ok

rma4ok Apr 12, 2016

@rsbondi @GianlucaGuarini @john-goldsmith
as @rogueg mentioned above

I'm actually moving this week, so not a ton of free time. Thanks for taking a stab at merging it

so it would be awesome if someone else could proceed with merging this PR

rma4ok commented Apr 12, 2016

@rsbondi @GianlucaGuarini @john-goldsmith
as @rogueg mentioned above

I'm actually moving this week, so not a ton of free time. Thanks for taking a stab at merging it

so it would be awesome if someone else could proceed with merging this PR

rogueg added some commits Feb 17, 2016

Unmount things when an if-attribute becomes false.
Before this change, we'd keep the old DOM, and simply avoid updating if when the if-attribute was false. It's more predictable to say that when DOM goes away, it's like it was never there to begin with. When the if-attribute becomes truthy again later, we should insert and mount new, clean DOM. This goes for custom tags as well as built-in stateful tags (like inputs).

To do it, I made if-attributes work similar to each. When parsed, it takes that element (plus all descendants), removes it from the DOM, and stores it as the pristine copy.

On update, if the value is true, we clone that pristine DOM, parse, and update it.

This change is breaking.
Refactor the code for setting keys on tags.
The code for setting (and removing) an object on a specified key was duplicated. This refactoring consolidates them.

This is currently used by name attributes (`id` and `name`) and the Tag.tags object.
Let unmount spread through the tree of expressions and tags, just lik…
…e update.

The reason is that now when an if-attribute becomes false, it unmounts just that part of the DOM tree using the unmountAll method.

This change makes Tag and If use the same unmounting code, which I think is clearer. Also, IMHO observable makes for difficult-to-debug internals.
Prevent each's internal state from leaking out and being modified.
`each` has an internal array that stores the tag instance of every item being rendered. To make these tags accessible from the parent (eg `parent.tags['child-tagName']`) someone assigned that internal array directly onto the parent.

Problem is, if that parent has other instances of that child tag, they'll start to conflict. If you have two loops over the same child element, they'll end up remove each others items.

This change uses the add/remove methods to modify tags instead. This was easy, but it means that parent.tags['child-tagName'] is less likely to reflect the actual order of elements in the DOM, and I  changed some tests to reflect this.

This is a breaking change (sorta).
Instantiate tags while parsing, so they're available to other express…
…ions.

This simplifies code by not needing a TagRef object between the parsing and update phases. It also means that we have a reference to the tag that can be given to expressions (see my next changed with NamedExp).
Refactor how the `name` and `id` attributes work.
The main goal here was to remove references from the parent when an element gets removed because of a false `if` attribute.

I really like the idea that expressions (just like tags) can now have an unmount method where they clean themselves up. For name expressions, that unmount method removes the reference. Simple.

I also made it so that `name` no longer affects the parent.tags collection. Before, you'd have this:
```
<custom-tag name='foo' />
parent.tags['custom-tag'] //=> undefined
parent.tags['foo'] //=> Tag
parent.foo //=> Tag
```

IMO, thats redundant and confusing. Now you get this:
```
parent.tags['custom-tag'] //=> Tag
parent.tags['foo'] //=> undefined
parent.foo //=> Tag
```

Since that's a breaking change, I had to modify a few tests. There are a few other cool things about this implementation:

* It handles if the name is an expression and changes.
* It handles `id` and `name` being different.
Fix each to work inside an if statement.
When the `if` is initially false, then each will miss the before-mount event. Instead, we need to remove the template dom immediately while parsing.

There was also a line that ensured the root pointed to the element that will eventually contain each item. I figured it was easier to just set this every time during update.
Allow setting the parent of a tag when mounting.
This lets you dynamically inject tags into the tree, which is especially useful for navigation.
Ignore Tag.update when the tag is unmounted.
It sometimes happens that you'll call update after a tag has been removed (xhr, timer, etc). Once it has been unmounted, doing an update is wasted work that might raise errors.
Always run update when mounting a tag.
It was inconsistent that update would only sometimes be run during a mount. This caused tags to not update when they were inside an if statement, because self.parent existed.

Instead, always run the update during mount. We also should only set DOM._tag and Tag.isMounted during the mount method, because those are used in other places to identify that the tag has mounted.
@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Apr 13, 2016

Member

This PR is updated and can be merged.

Member

rogueg commented Apr 13, 2016

This PR is updated and can be merged.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 13, 2016

Member

well done @rogueg 👏

Member

GianlucaGuarini commented Apr 13, 2016

well done @rogueg 👏

@GianlucaGuarini GianlucaGuarini merged commit 224e1e9 into riot:next Apr 13, 2016

@GianlucaGuarini GianlucaGuarini referenced this pull request Apr 13, 2016

Closed

Riot 3.0.0 roadmap #1694

14 of 16 tasks complete
@@ -171,10 +173,7 @@ function Tag(impl, conf, innerHTML) {
// parse layout after init. fn may calculate args for nested custom tags
parseExpressions(dom, self, expressions)
// unmount automatically when our parent does
if (parent) parent.on('unmount', self.unmount)

This comment has been minimized.

@rsbondi

rsbondi Apr 15, 2016

Member

Where is this handled now? It seems to have created a problem with dynamic data-is tags now go out of sync in the parent.tags. It seems to partially work and all test pass if I add here parent.on('unmount', function() { expr.tag.unmount()}).

You can see the issue here, just hit the add button, then remove what you added and look at the console, you will see {string: Array[2]} but it should return to {string: Tag}. The above takes care of part of the problem for simple removals.

There is another issue when a tag is swapped to a different type. The code here used to take care of that, but now this code does not get reached. Any insight on how this can be handled would be appreciated.

EDIT: the second issue seems to be fixed by changing expr.tag.opts.riotTag to expr.tag.tagName, so does this seem reasonable or should I look somewhere else?

@rsbondi

rsbondi Apr 15, 2016

Member

Where is this handled now? It seems to have created a problem with dynamic data-is tags now go out of sync in the parent.tags. It seems to partially work and all test pass if I add here parent.on('unmount', function() { expr.tag.unmount()}).

You can see the issue here, just hit the add button, then remove what you added and look at the console, you will see {string: Array[2]} but it should return to {string: Tag}. The above takes care of part of the problem for simple removals.

There is another issue when a tag is swapped to a different type. The code here used to take care of that, but now this code does not get reached. Any insight on how this can be handled would be appreciated.

EDIT: the second issue seems to be fixed by changing expr.tag.opts.riotTag to expr.tag.tagName, so does this seem reasonable or should I look somewhere else?

This comment has been minimized.

@GianlucaGuarini

GianlucaGuarini Apr 15, 2016

Member

@rsbondi this means we need more tests to cover also these cases. Could you please make a pull request adding these tests? I guess it will be trivial to fix these issues having more detailed specs.

@GianlucaGuarini

GianlucaGuarini Apr 15, 2016

Member

@rsbondi this means we need more tests to cover also these cases. Could you please make a pull request adding these tests? I guess it will be trivial to fix these issues having more detailed specs.

This comment has been minimized.

@GianlucaGuarini

GianlucaGuarini Apr 15, 2016

Member

I think we can also start cleaning up the tests, making several folders targeting specific riot directives. Who wants to take the lead on this? I can do it eventually together with the new rollup build in the middle of May but I would prefer someone could already start with it

@GianlucaGuarini

GianlucaGuarini Apr 15, 2016

Member

I think we can also start cleaning up the tests, making several folders targeting specific riot directives. Who wants to take the lead on this? I can do it eventually together with the new rollup build in the middle of May but I would prefer someone could already start with it

This comment has been minimized.

@rogueg

rogueg Apr 17, 2016

Member

Yeah, a test for this case would be great. I peeked at the code, and have an idea of how to fix it.

@rogueg

rogueg Apr 17, 2016

Member

Yeah, a test for this case would be great. I peeked at the code, and have an idea of how to fix it.

@GianlucaGuarini GianlucaGuarini referenced this pull request Jan 9, 2017

Closed

IF attr will mount/unmount the el ?? #2204

1 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment