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

Leaking (the new) if binding introduced in v1.5.2 #328

Closed
huochunpeng opened this Issue Oct 24, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@huochunpeng
Contributor

huochunpeng commented Oct 24, 2017

I'm submitting a bug report

  • Library Version:
    1.5.2

Please tell us about your environment:

  • Operating System:
    OSX 10.x

  • Node Version:
    8.5.0

  • NPM Version:
    5.5.1

  • JSPM OR Webpack AND Version
    CLI

  • Browser:
    all

  • Language:
    ESNext

Current behavior:
https://github.com/huochunpeng/ifff

A leaking if binding behind nested if binding

Run the test app, just click next and next, all conditional DOM are not removed.

Pretty sure this is introduced by #324 which attempt to fix another issue on the new if/else implementation.

This issue doesn't exist on v1.5.1 before #324 fix.

Expected/desired behavior:

bring back old if.bind please...
The new if/else doesn't justify the pain...

  • What is the expected behavior?

  • What is the motivation / use case for changing the behavior?

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Oct 24, 2017

Contributor

@EisenbergEffect @jdanyow @jods4 I am getting really really frustrated of this new if/else binding implementation. The new edge cases are really really hard to debug and repeat.

The new if/else implementation changed too much, created lots of new edge cases (just read through your issue list). The result is breaking everybody's production app. I have to say this is not a production ready feature.

Please please revert back to old if binding implementation. And put this new implementation into a beta/preview release for people to try.

I suggest to maintain a conservative Aurelia stable release, and only test experimental features on beta/preview release.

I am back to v1.4.0 again.

Contributor

huochunpeng commented Oct 24, 2017

@EisenbergEffect @jdanyow @jods4 I am getting really really frustrated of this new if/else binding implementation. The new edge cases are really really hard to debug and repeat.

The new if/else implementation changed too much, created lots of new edge cases (just read through your issue list). The result is breaking everybody's production app. I have to say this is not a production ready feature.

Please please revert back to old if binding implementation. And put this new implementation into a beta/preview release for people to try.

I suggest to maintain a conservative Aurelia stable release, and only test experimental features on beta/preview release.

I am back to v1.4.0 again.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Oct 24, 2017

Member

Did you test out the version that was released last night? That fixed a number of issues.

Member

EisenbergEffect commented Oct 24, 2017

Did you test out the version that was released last night? That fixed a number of issues.

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Oct 24, 2017

Contributor

I am talking about v1.5.2 you just released.

Contributor

huochunpeng commented Oct 24, 2017

I am talking about v1.5.2 you just released.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Oct 24, 2017

Member

I see. Just confirmed that myself. Thanks.

Member

EisenbergEffect commented Oct 24, 2017

I see. Just confirmed that myself. Thanks.

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Oct 24, 2017

Contributor

This is a new issue introduced by a fix.

Contributor

huochunpeng commented Oct 24, 2017

This is a new issue introduced by a fix.

@huochunpeng huochunpeng changed the title from Leading (the new) if binding introduced in v1.5.2 to Leaking (the new) if binding introduced in v1.5.2 Oct 24, 2017

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Oct 24, 2017

Contributor

If you wonder what I mean by "Leading", it's a typo of "Leaking". Sorry about that.

Contributor

huochunpeng commented Oct 24, 2017

If you wonder what I mean by "Leading", it's a typo of "Leaking". Sorry about that.

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Oct 24, 2017

Contributor

Update: confirmed this issue still occurs in latest v1.5.3.

Contributor

huochunpeng commented Oct 24, 2017

Update: confirmed this issue still occurs in latest v1.5.3.

mjwwit added a commit to mjwwit/templating-resources that referenced this issue Oct 25, 2017

fix(if): fix nested if bindings
- Revert (flawed) fix from aurelia#324
- Force rebind of view (if not bound) if the if-binding is rebound and condition is true
- Fix unit-tests for repeat, which were failing because of work done in aurelia#303

Closes aurelia#328

jods4 added a commit to jods4/templating-resources that referenced this issue Oct 25, 2017

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Oct 25, 2017

Contributor

@huochunpeng 1.5.3 was a PR for a problem with else, not if.
Sorry it's been that bad, I have had a good look at it and I think I have a proper fix ready in the PR above.

Contributor

jods4 commented Oct 25, 2017

@huochunpeng 1.5.3 was a PR for a problem with else, not if.
Sorry it's been that bad, I have had a good look at it and I think I have a proper fix ready in the PR above.

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Oct 25, 2017

Contributor

Update: @jods4 your PR fixed this issue.

Contributor

huochunpeng commented Oct 25, 2017

Update: @jods4 your PR fixed this issue.

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Oct 26, 2017

Contributor

@huochunpeng Thanks for taking the time to try it! It's good to get confirmation before publishing.

Contributor

jods4 commented Oct 26, 2017

@huochunpeng Thanks for taking the time to try it! It's good to get confirmation before publishing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment