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

Repeat binding iterates an extra time for each item in array #575

Open
Kukks opened this Issue Oct 25, 2017 · 26 comments

Comments

Projects
None yet
9 participants
@Kukks

Kukks commented Oct 25, 2017

I'm submitting a bug report

  • Library Version:
    1.6.0

Please tell us about your environment:

  • Operating System:
    Win 10

  • Node Version:
    6.11.4

  • NPM Version:
    3.10.10

  • JSPM OR Webpack AND Version
    webpack 3.6.0

  • Browser:
    Chrome

  • Language:
    TS 2.5.3

Current behavior:
When adding an extra item to an observed array, the bindings on the repeat seem to break.
an array with length 2 would repeat 4 times, with the last two items being null.

<div repeat.for="item of items">
${$index} -- ${item}
</div>
<div>0 -- a</div>
<div>1 -- b</div>
<div>2 -- </div>
<div>3 -- </div>

My current workaround is ugly:
if.bind="$index<items.length"

@bigopon

This comment has been minimized.

Show comment
Hide comment
@bigopon

bigopon Oct 25, 2017

Member

It's working as expected for me. Can you describe how to reproduce the error ?

Member

bigopon commented Oct 25, 2017

It's working as expected for me. Can you describe how to reproduce the error ?

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Oct 25, 2017

@bigopon I wish I could, it only started happening on my colleague's workstation from a fresh npm install. Our solution is a bit too massive to easily dissect and find what makes it break like this.
Seems like the repeat binding breaks on splices. Possible related error here: #574

Kukks commented Oct 25, 2017

@bigopon I wish I could, it only started happening on my colleague's workstation from a fresh npm install. Our solution is a bit too massive to easily dissect and find what makes it break like this.
Seems like the repeat binding breaks on splices. Possible related error here: #574

@StrahilKazlachev

This comment has been minimized.

Show comment
Hide comment
@StrahilKazlachev

StrahilKazlachev Oct 25, 2017

Contributor

@Kukks were there any lock files prior to the clean install?

Contributor

StrahilKazlachev commented Oct 25, 2017

@Kukks were there any lock files prior to the clean install?

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Nov 6, 2017

package lock and bumping the versions was the issue. Thanks for the help!

Kukks commented Nov 6, 2017

package lock and bumping the versions was the issue. Thanks for the help!

@Kukks Kukks closed this Nov 6, 2017

jdanyow added a commit that referenced this issue Nov 13, 2017

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Jun 13, 2018

This has started appearing again. Tried all sorts of annoying tactics with package locks and version bumping..
#574 (comment)

Kukks commented Jun 13, 2018

This has started appearing again. Tried all sorts of annoying tactics with package locks and version bumping..
#574 (comment)

@Kukks Kukks reopened this Jun 13, 2018

@bigopon

This comment has been minimized.

Show comment
Hide comment
@bigopon

bigopon Jun 20, 2018

Member

Hmm it's not easy to track down, I have updating through many version and haven't gotten into that bad state of the repeat. Can you please share a repro or some more info @Kukks ?

Member

bigopon commented Jun 20, 2018

Hmm it's not easy to track down, I have updating through many version and haven't gotten into that bad state of the repeat. Can you please share a repro or some more info @Kukks ?

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Jun 20, 2018

Just an example of using .push on the array, with a .length print at the bottom:
https://gyazo.com/f5def4c9e4d8897e866a26f48b904309

Kukks commented Jun 20, 2018

Just an example of using .push on the array, with a .length print at the bottom:
https://gyazo.com/f5def4c9e4d8897e866a26f48b904309

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Jun 20, 2018

I haven't been able to reproduce outside of this massive company project. Anything specific you want from my end that could help?

Kukks commented Jun 20, 2018

I haven't been able to reproduce outside of this massive company project. Anything specific you want from my end that could help?

@bigopon

This comment has been minimized.

Show comment
Hide comment
@bigopon

bigopon Jun 20, 2018

Member

Maybe the specific block that updating the array should do

Member

bigopon commented Jun 20, 2018

Maybe the specific block that updating the array should do

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Jun 20, 2018

This is pretty much it:

<table class="table">
          <thead>
            <th t="name">Name</th>
            <th></th>
          </thead>
          <tbody>
          <resource-data-source resource="TranslationCultures"
                                view-model.ref="translationCulturesDS"></resource-data-source>
          <tr repeat.for="chosenTranslation of chosenTranslations">
            <td class="d-flex">
              <h-options
                class="col-md-6"
                read-only.bind="!editMode"
                if.bind="translationCulturesDS && translationCulturesDS.metadata"
                translate.bind="false"
                value-identifier.bind="chosenTranslations[$index]"
                allow-clear.bind="false"
                load-remote-options.bind="translationCulturesDS.loadSelectRemoteData"
                load-single-option.bind="translationCulturesDS.loadSingleRemoteData"
                prop-display.bind="translationCulturesDS.metadata.optionSettings.name"
                prop-id.bind="translationCulturesDS.metadata.optionSettings.id"></h-options>
            </td>
            <td class="text-right">
              <button class="btn-sm btn-link text-danger" h-button click.delegate="chosenTranslations.splice($index, 1)" if.bind="editMode">
                <h-icon name="trash-o"></h-icon>
                <span t="remove">Remove</span>
              </button>
            </td>
          </tr>
          <tr>
            <td>
              <h-spinner show.bind="loading"></h-spinner>
            </td>
            <td class="text-right">
              <button class="btn-link text-dark" h-button click.delegate="chosenTranslations.push(null)" if.bind="editMode">
                <h-icon name="plus-circle"></h-icon> ${'add' & t}
              </button>
            </td>
          </tr>
          </tbody>
          <tfoot>
            <tr>
              <td>${chosenTranslations.length}</td>
            </tr>
          </tfoot>
        </table>

Kukks commented Jun 20, 2018

This is pretty much it:

<table class="table">
          <thead>
            <th t="name">Name</th>
            <th></th>
          </thead>
          <tbody>
          <resource-data-source resource="TranslationCultures"
                                view-model.ref="translationCulturesDS"></resource-data-source>
          <tr repeat.for="chosenTranslation of chosenTranslations">
            <td class="d-flex">
              <h-options
                class="col-md-6"
                read-only.bind="!editMode"
                if.bind="translationCulturesDS && translationCulturesDS.metadata"
                translate.bind="false"
                value-identifier.bind="chosenTranslations[$index]"
                allow-clear.bind="false"
                load-remote-options.bind="translationCulturesDS.loadSelectRemoteData"
                load-single-option.bind="translationCulturesDS.loadSingleRemoteData"
                prop-display.bind="translationCulturesDS.metadata.optionSettings.name"
                prop-id.bind="translationCulturesDS.metadata.optionSettings.id"></h-options>
            </td>
            <td class="text-right">
              <button class="btn-sm btn-link text-danger" h-button click.delegate="chosenTranslations.splice($index, 1)" if.bind="editMode">
                <h-icon name="trash-o"></h-icon>
                <span t="remove">Remove</span>
              </button>
            </td>
          </tr>
          <tr>
            <td>
              <h-spinner show.bind="loading"></h-spinner>
            </td>
            <td class="text-right">
              <button class="btn-link text-dark" h-button click.delegate="chosenTranslations.push(null)" if.bind="editMode">
                <h-icon name="plus-circle"></h-icon> ${'add' & t}
              </button>
            </td>
          </tr>
          </tbody>
          <tfoot>
            <tr>
              <td>${chosenTranslations.length}</td>
            </tr>
          </tfoot>
        </table>
@bigopon

This comment has been minimized.

Show comment
Hide comment
@bigopon

bigopon Jun 20, 2018

Member

How you are adding element to chosenTranslations in your view model when clicking on Add button ?

Member

bigopon commented Jun 20, 2018

How you are adding element to chosenTranslations in your view model when clicking on Add button ?

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Jun 20, 2018

chosenTranslations.push(null); straight from the html
I have other scenarios where I get the same effect from a view model x.push

Kukks commented Jun 20, 2018

chosenTranslations.push(null); straight from the html
I have other scenarios where I get the same effect from a view model x.push

@VagyokC4

This comment has been minimized.

Show comment
Hide comment
@VagyokC4

VagyokC4 Jul 6, 2018

I'm experiencing this problem as well... Can we get this issue escalated and resolved?

Animated GIF

VagyokC4 commented Jul 6, 2018

I'm experiencing this problem as well... Can we get this issue escalated and resolved?

Animated GIF

@doktordirk

This comment has been minimized.

Show comment
Hide comment
@doktordirk

doktordirk Jul 7, 2018

can you double check that it's not pushed twice? maybe the event is fired twice for some reason

doktordirk commented Jul 7, 2018

can you double check that it's not pushed twice? maybe the event is fired twice for some reason

@Kukks

This comment has been minimized.

Show comment
Hide comment
@Kukks

Kukks Jul 7, 2018

Kukks commented Jul 7, 2018

@doktordirk

This comment has been minimized.

Show comment
Hide comment
@doktordirk

doktordirk Jul 7, 2018

@Kukks hard to find anything on gitter. can you repost here or in discourse?

doktordirk commented Jul 7, 2018

@Kukks hard to find anything on gitter. can you repost here or in discourse?

@michaelsmithson

This comment has been minimized.

Show comment
Hide comment
@michaelsmithson

michaelsmithson Jul 25, 2018

We had something similar happening in our project today, where an array push was causing duplicate items. Looking at the final compiled code it appears we have ended up with two different versions of the aurelia-binding library imported in our solution (1.6 and 2.1). (We are using jspm)

The binding library patches the push method on the array prototype with a version that adds a change record. With two versions of the library included this being patched twice, it resulted in two changes being recorded for every push.

michaelsmithson commented Jul 25, 2018

We had something similar happening in our project today, where an array push was causing duplicate items. Looking at the final compiled code it appears we have ended up with two different versions of the aurelia-binding library imported in our solution (1.6 and 2.1). (We are using jspm)

The binding library patches the push method on the array prototype with a version that adds a change record. With two versions of the library included this being patched twice, it resulted in two changes being recorded for every push.

@stuartbale

This comment has been minimized.

Show comment
Hide comment
@stuartbale

stuartbale Jul 25, 2018

@michaelsmithson - when you say "binding library" do you specifically mean aurelia-binding (https://www.npmjs.com/package/aurelia-binding) or something else?
Also, what steps did you take to isolate this please?

stuartbale commented Jul 25, 2018

@michaelsmithson - when you say "binding library" do you specifically mean aurelia-binding (https://www.npmjs.com/package/aurelia-binding) or something else?
Also, what steps did you take to isolate this please?

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Jul 25, 2018

Contributor

Here is my guess.

Probably this is the way how webpack works, it tries to mimic nodejs behaviour.

Previously @EisenbergEffect released new parser in aurelia-binding v2 to make it a safe opt-in. But since then, aurelia core libs have been updated to depend on v2.

But if you have some 3rd party plugin (very likely) still asks aurelia-binding ^1.x.x in the package.json, you end up with 2 versions of binding in your node_modules (one direct, another one under the plugin folder).

webpack tries to respect the plugin's dependency, hence bundled 2 versions of aurelia-binding.

This also answers why I didn't see this nasty issue, as I use cli+requirejs built, it only bundles one version of every libs.

Contributor

huochunpeng commented Jul 25, 2018

Here is my guess.

Probably this is the way how webpack works, it tries to mimic nodejs behaviour.

Previously @EisenbergEffect released new parser in aurelia-binding v2 to make it a safe opt-in. But since then, aurelia core libs have been updated to depend on v2.

But if you have some 3rd party plugin (very likely) still asks aurelia-binding ^1.x.x in the package.json, you end up with 2 versions of binding in your node_modules (one direct, another one under the plugin folder).

webpack tries to respect the plugin's dependency, hence bundled 2 versions of aurelia-binding.

This also answers why I didn't see this nasty issue, as I use cli+requirejs built, it only bundles one version of every libs.

@stuartbale

This comment has been minimized.

Show comment
Hide comment
@stuartbale

stuartbale Jul 25, 2018

Ah yes, i see that my:
aurelia-plugins-google-recaptcha
'requires' aurelia-binding ^1.7.1
I will see if I can fix it somehow.

stuartbale commented Jul 25, 2018

Ah yes, i see that my:
aurelia-plugins-google-recaptcha
'requires' aurelia-binding ^1.7.1
I will see if I can fix it somehow.

@michaelsmithson

This comment has been minimized.

Show comment
Hide comment
@michaelsmithson

michaelsmithson Jul 25, 2018

@stuartbale yep, I do mean the aurelia-binding library.
To find this I was stepping through the code in the chrome debugger, and noticed this weird behaviour inside the binding library push function. I then stuck a breakpoint before it patched the push method and noticed that when I inspected the current Array.prototype.push method that it was not the native function (push() { [native code] }) but rather appeared already to be the aurelia patched version. At this point I hunted around and found that there were two versions of aurelia-binding being included by jspm.

michaelsmithson commented Jul 25, 2018

@stuartbale yep, I do mean the aurelia-binding library.
To find this I was stepping through the code in the chrome debugger, and noticed this weird behaviour inside the binding library push function. I then stuck a breakpoint before it patched the push method and noticed that when I inspected the current Array.prototype.push method that it was not the native function (push() { [native code] }) but rather appeared already to be the aurelia patched version. At this point I hunted around and found that there were two versions of aurelia-binding being included by jspm.

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Jul 25, 2018

Contributor

The new resurgence of this isssue, is apparently different from the original issue created by @Kukks.
The original issue was demonstrated in gistrun, which is an aurelia bug. It has been fixed.

The resurgence is caused by which I guess the webpack packed two versions of aurelia-binding.

It's confirmed.

I can reproduce this issue with cli+webpack, just use plugin aurelia-plugins-google-recaptcha (which requires aurelia-binding ^1.7.1).

Not only the repeat bug appear, but also DuplicatePackageCheckerPlugin complains

WARNING in aurelia-binding
  Multiple versions of aurelia-binding found:
    1.7.1 ./~/aurelia-plugins-google-recaptcha/~/aurelia-binding
    2.1.2 ./~/aurelia-binding

Bingo.

Based on @michaelsmithson's experience, jspm does the same.

Contributor

huochunpeng commented Jul 25, 2018

The new resurgence of this isssue, is apparently different from the original issue created by @Kukks.
The original issue was demonstrated in gistrun, which is an aurelia bug. It has been fixed.

The resurgence is caused by which I guess the webpack packed two versions of aurelia-binding.

It's confirmed.

I can reproduce this issue with cli+webpack, just use plugin aurelia-plugins-google-recaptcha (which requires aurelia-binding ^1.7.1).

Not only the repeat bug appear, but also DuplicatePackageCheckerPlugin complains

WARNING in aurelia-binding
  Multiple versions of aurelia-binding found:
    1.7.1 ./~/aurelia-plugins-google-recaptcha/~/aurelia-binding
    2.1.2 ./~/aurelia-binding

Bingo.

Based on @michaelsmithson's experience, jspm does the same.

@stuartbale

This comment has been minimized.

Show comment
Hide comment
@stuartbale

stuartbale Jul 25, 2018

@huochunpeng I concur. I installed DuplicatePackageCheckerPlugin and saw the same result as you.

stuartbale commented Jul 25, 2018

@huochunpeng I concur. I installed DuplicatePackageCheckerPlugin and saw the same result as you.

@fkleuver

This comment has been minimized.

Show comment
Hide comment
@fkleuver

fkleuver Jul 25, 2018

Member

Yep, this will keep happening as long as these 3 conditions are met:

  • aurelia-binding@^2.0.0 is pulled in as a result of any main dependency allowing it (which is approximately the whole framework at least)
  • aurelia-binding@^1.0.0 is pulled in as a result of another dependency disallowing it (which is a large portion of the plugin landscape)
  • webpack and jspm not offering a means to override the dependency resolution tree

At the moment, the best thing to do I believe is to try and get all those plugins updated. They can be found by looking at https://www.npmjs.com/package/aurelia-binding / dependents. I'll start sending some PRs early next week. Anyone feel free to chime in and help out.

Member

fkleuver commented Jul 25, 2018

Yep, this will keep happening as long as these 3 conditions are met:

  • aurelia-binding@^2.0.0 is pulled in as a result of any main dependency allowing it (which is approximately the whole framework at least)
  • aurelia-binding@^1.0.0 is pulled in as a result of another dependency disallowing it (which is a large portion of the plugin landscape)
  • webpack and jspm not offering a means to override the dependency resolution tree

At the moment, the best thing to do I believe is to try and get all those plugins updated. They can be found by looking at https://www.npmjs.com/package/aurelia-binding / dependents. I'll start sending some PRs early next week. Anyone feel free to chime in and help out.

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Jul 26, 2018

Contributor

FYI
Temporary solution for duplicated aurelia-binding in webpack bundle:

  1. use https://github.com/darrenscerri/duplicate-package-checker-webpack-plugin to detect duplicates.
  2. using alias to force one version of aurelia-binding

webpack.config.js

const DuplicatePackageCheckerPlugin = require('duplicate-package-checker-webpack-plugin');
// ...
module.exports = ({production, server, extractCss, coverage, analyze} = {}) => ({
  resolve: {
    extensions: ['.js'],
    modules: [srcDir, 'node_modules'],
    // Enforce single aurelia-binding
    alias: {
      'aurelia-binding': path.resolve(__dirname, 'node_modules/aurelia-binding')
    }
  },
  // ...
  plugins: [
    // Detect and warn about duplicates
    new DuplicatePackageCheckerPlugin(),
    new AureliaPlugin(),
    // ...
  ]
});
Contributor

huochunpeng commented Jul 26, 2018

FYI
Temporary solution for duplicated aurelia-binding in webpack bundle:

  1. use https://github.com/darrenscerri/duplicate-package-checker-webpack-plugin to detect duplicates.
  2. using alias to force one version of aurelia-binding

webpack.config.js

const DuplicatePackageCheckerPlugin = require('duplicate-package-checker-webpack-plugin');
// ...
module.exports = ({production, server, extractCss, coverage, analyze} = {}) => ({
  resolve: {
    extensions: ['.js'],
    modules: [srcDir, 'node_modules'],
    // Enforce single aurelia-binding
    alias: {
      'aurelia-binding': path.resolve(__dirname, 'node_modules/aurelia-binding')
    }
  },
  // ...
  plugins: [
    // Detect and warn about duplicates
    new DuplicatePackageCheckerPlugin(),
    new AureliaPlugin(),
    // ...
  ]
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment