aurelia / framework

The Aurelia 1 framework entry point, bringing together all the required sub-modules of Aurelia.
MIT License
11.75k stars 623 forks source link

repeat.for renders same item twice #830

Closed powerbuoy closed 5 years ago

powerbuoy commented 7 years ago

I have a strange issue where when I try to push an empty object to an array that is rendered in my view - the newly pushed element is rendered twice.

The duplicate element is in fact the exact same element as the previous one (if I change the first the second also changes).

I'm unable to reproduce this in a GistRun (perhaps version differences) - but I have managed to reproduce it in a completely new Aurelia CLI project with the following code:

app.js

export class App {
    constructor () {
        this.items = [];
        this.editing = false;
    }

    // Toggle the edit form
    toggleEditing () {
        this.editing = !this.editing;

        // Also add a new item
        if (this.editing) {
            this.items.push({}); // NOTE: This is where things go wrong
        }
    }
}

And this view:

app.html

<template>

    <template if.bind="!editing">
        <h2>${items.length} items!</h2>
        <ul>
            <li repeat.for="item of items">
                ${item.name}
            </li>
        </ul>
        <button click.delegate="toggleEditing()">Edit</button>
    </template>

    <template if.bind="editing">
        <ul>
            <li repeat.for="item of items">
                <input type="text" value.bind="item.name">
            </li>
        </ul>
        <button click.delegate="toggleEditing()">Save</button>
    </template>

</template>

My package.json looks like this (for version info):

package.json

{
  "name": "autest",
  "description": "An Aurelia client application.",
  "version": "0.1.0",
  "repository": {
    "type": "???",
    "url": "???"
  },
  "license": "MIT",
  "dependencies": {
    "aurelia-bootstrapper": "^2.1.1",
    "aurelia-animator-css": "^1.0.2",
    "bluebird": "^3.4.1",
    "requirejs": "^2.3.2",
    "text": "github:requirejs/text#latest"
  },
  "peerDependencies": {},
  "devDependencies": {
    "aurelia-cli": "^0.31.3",
    "aurelia-testing": "^1.0.0-beta.3.0.1",
    "aurelia-tools": "^1.0.0",
    "gulp": "github:gulpjs/gulp#4.0",
    "minimatch": "^3.0.2",
    "through2": "^2.0.1",
    "uglify-js": "^3.0.19",
    "vinyl-fs": "^2.4.3",
    "babel-eslint": "^6.0.4",
    "babel-plugin-syntax-flow": "^6.8.0",
    "babel-plugin-transform-decorators-legacy": "^1.3.4",
    "babel-plugin-transform-es2015-modules-amd": "^6.8.0",
    "babel-plugin-transform-es2015-modules-commonjs": "^6.10.3",
    "babel-plugin-transform-flow-strip-types": "^6.8.0",
    "babel-preset-es2015": "^6.13.2",
    "babel-preset-stage-1": "^6.5.0",
    "babel-polyfill": "^6.9.1",
    "babel-register": "^6.24.0",
    "gulp-babel": "^6.1.2",
    "gulp-eslint": "^2.0.0",
    "gulp-htmlmin": "^3.0.0",
    "html-minifier": "^3.2.3",
    "jasmine-core": "^2.4.1",
    "karma": "^0.13.22",
    "karma-chrome-launcher": "^2.2.0",
    "karma-jasmine": "^1.0.2",
    "karma-sourcemap-loader": "^0.3.7",
    "karma-babel-preprocessor": "^6.0.1",
    "browser-sync": "^2.13.0",
    "connect-history-api-fallback": "^1.2.0",
    "debounce": "^1.0.2",
    "gulp-changed-in-place": "^2.0.3",
    "gulp-plumber": "^1.1.0",
    "gulp-rename": "^1.2.2",
    "gulp-sourcemaps": "^2.0.0-alpha",
    "gulp-notify": "^2.2.0",
    "gulp-watch": "^4.3.11"
  }
}

Maybe it's something obvious I've missed? But it seems like a genuine bug to me.

I'm attaching my example app as well which exhibits the bug and can be run in Firefox directly. Just click "Edit" and two input fields will be shown instead of one - changing one actually changes both.

autest.zip

bigopon commented 7 years ago

If you are using templating-resources@1.5.1, possibly it's related to aurelia/templating-resources#317

powerbuoy commented 7 years ago

@bigopon I am indeed using 1.5.1. I'm not sure they're related though?

I've simplified my example code slightly. And FYI I'm using Aurelia CLI 0.31.3 (which I believe is the latest version?)

jwx commented 7 years ago

Do you get the same issue if you remove the if-part from the example?

powerbuoy commented 7 years ago

@jwx I do not. Removing the if solves the issue. Also switching to show instead of if solves it - however, show does not appear to work with <template>.

Edit: I guess it might be the exact same issue then?

jwx commented 7 years ago

@powerbuoy Then the problem is probably with if, so yeah, at least related to it. There's a fix in that one that you could try and see if it helps.

powerbuoy commented 7 years ago

Ok, honestly I'm not sure how I would implement the fix? You mean the ifCore override right?

I went ahead and updated all Aurelia packages today (I basically deleted node_modules and just reinstalled everything with my package.json pointing to the latest versions of all packages). I've also got the latest version of node and npm.

However, I still get the same issue as above and now new issues are introduced that also seem related to if.bind. When toggling the visibility of a <template> things (bindings) simply stop working. My app is littered with this code so it isn't really feasible for me to change it all now.

I'm guessing this is the same issue as mentioned in https://github.com/aurelia/templating-resources/issues/317 and https://github.com/aurelia/templating-resources/issues/315

How does one go about rolling back to a previous version? Most of the Aurelia packages aren't in package.json so not sure how their version numbers are controlled?

mreiche commented 7 years ago

How does one go about rolling back to a previous version? Most of the Aurelia packages aren't in package.json so not sure how their version numbers are controlled?

I've spent hours trying to rollback to a previous version with JSPM. Without luck. Problem was, that I'm not able to LOCK a specific version of templating-resources and it's gonna overridden by the dependency tree: aurelia-materialize-bridge -> aurelia-framework -> aurelia-templating -> aurelia-templating-resources.

If you figured out how to rollback, tell me, I will really appreciate that!

powerbuoy commented 7 years ago

I've pretty much swapped out all my if.bind:s to show.bind:s instead and while it meant I had to re-code some stuff that will require rigorous testing it seems to be working at first glance for now at least.

Here's hoping for an official patch soon :)

mreiche commented 7 years ago

I've pretty much swapped out all my if.bind:s to show.bind:s instead and while it meant I had to re-code some stuff that will require rigorous testing it seems to be working at first glance for now at least.

Yes, but that doesn't help for repeat.

powerbuoy commented 7 years ago

I see there's a fix for both https://github.com/aurelia/templating-resources/issues/317 and https://github.com/aurelia/templating-resources/issues/315 out. However, my repeat.for issue, as well as the issues I have with if.bind are not solved by the new release.

I managed to reproduce my issue in gist.run now as well; https://gist.run/?id=af8b192abd4ce89a62fe1383dba79d93

Changing from if.bind to show.bind solves it there too. I'm not 100% sure that gist is using the latest version of everything, but this same thing happens locally with everything updated (aurelia-templating@1.6.0)

StrahilKazlachev commented 7 years ago

@powerbuoy Not released yet.

powerbuoy commented 7 years ago

Ah ok, my bad :) I guess the fix is in templating-resources?

StrahilKazlachev commented 7 years ago

Yes.

dkent600 commented 7 years ago

This is happening to me, but has nothing to do with if or show:

The result is that the content is rendered twice for each array item that is added using myArray.push(newItem) after the view has been bound.

What is really weird is that when I do a production build, the bug goes away.

<template>
<div repeat.for='item of myArray'>
    Item
</div> 
</template>

I am on aurelia-templating-resources v1.5.3

dkent600 commented 7 years ago

OK, referring to my repro case just above ^^^, I figured out how to make the bug go away, even in my dev build. First, note that this applies to aurelia-typescript-webpack. The change is in webpack.config.js.

The original, that works, looks like this in the entry property:

entry: {
    app: ['aurelia-bootstrapper'],
    vendor: ['bluebird', 'jquery', 'bootstrap'],
  }

What doesn't work, and causes this bug to happen in the development build, is the following:

    entry: {
      app: ['aurelia-bootstrapper'],
      vendor: [
        'bluebird', 
        'aurelia-templating', 
        'aurelia-binding', 
        'aurelia-router', 
        'aurelia-templating-binding', 
        'aurelia-polyfills',
        'aurelia-event-aggregator',
        'bootstrap', 
        'jquery',
        ],
  }
esmoore68 commented 7 years ago

I'm still seeing this issue in my app, using aurelia-templating-resources@1.5.4. This bug is causing me to have to jump through all kinds of hoops to avoid.

bigopon commented 7 years ago

@esmoore68 does it occur with templating-resources@1.4.0 ?

@jods4 If it's ok with 1.4.0, then i think repeat has some issue, not the if

StrahilKazlachev commented 7 years ago

I just tried a CLI build with no problems - aurelia-templating-resources@1.5.4. @esmoore68 please provide a repro of the problem you are facing.

dkent600 commented 7 years ago

@StrahilKazlachev You might be able to repro with a webpack.config such as I described above.

esmoore68 commented 7 years ago

@StrahilKazlachev, the actual cases (there are many) in my app might be difficult to distill, but I can easily recreate using the test case created by @powerbuoy above.

@bigopon- It still happens with templating-resources@1.4.0.

Aurelia entries from my packages.json:

    "aurelia-animator-css": "^1.0.1",
    "aurelia-binding": "^1.5.0",
    "aurelia-bootstrapper": "^2.1.1",
    "aurelia-dialog": "^1.0.0-rc.1.0.3",
    "aurelia-event-aggregator": "^1.0.1",
    "aurelia-fetch-client": "^1.1.1",
    "aurelia-http-client": "^1.0.4",
    "aurelia-templating": "^1.6.0",
    "aurelia-templating-resources": "^1.4.0",
    "aurelia-validation": "^1.0.0",

I am using Typescript and Webpack.

related settings from my webpack.config:

const {AureliaPlugin, ModuleDependenciesPlugin } = require("aurelia-webpack-plugin");

module.exports = {
  entry: "aurelia-bootstrapper",
  },
BlueManiac commented 7 years ago

I also have this problem using a simple

<tr repeat.for="op of operations">
  <td>${op.operationId}</td>
</tr>

The resulting html after pushing two items:

<tr>
    <td>1</td>
</tr>
<tr>
    <td>2</td>
</tr>
<tr>
    <td></td>
</tr>
<tr>
    <td></td>
</tr>

So the "duplicate" rows have no contents.

using: "aurelia-bootstrapper": "^2.0.1", "aurelia-fetch-client": "^1.1.3", "aurelia-framework": "^1.1.5", "aurelia-loader-webpack": "^2.0.0", "aurelia-pal": "^1.4.0", "aurelia-router": "^1.4.0", "aurelia-templating-resources": "^1.5.4",

bigopon commented 7 years ago

I still cant reproduce it. @esmoore68 @BlueManiac can you confirm that your templating-resources@1.5.4 is used ? Maybe check inside node_modules/aurelia-bootstrapper to see if there is any node_modules with templating-resources there.

StrahilKazlachev commented 7 years ago

You can do npm ls aurelia-templating-resources.

BlueManiac commented 7 years ago

Seems to be correct. I removed dist and rebuilt the application, same thing.

npm ls aurelia-templating-resources

+-- aurelia-bootstrapper@2.1.1
| `-- aurelia-templating-resources@1.5.4  deduped
`-- aurelia-templating-resources@1.5.4

An interesting note is that if I update the view using HMR the display problem goes away until I add more rows.

esmoore68 commented 7 years ago

npm ls aurelia-templating-resources -- aurelia-templating-resources@1.5.4`

I verified that the version of aurelia-templating-resources being built in to my webpack bundle includes the code changed in the fix that @jods4 made to this module that I think was supposed to address this issue?

https://github.com/aurelia/templating-resources/commit/9548e16bc9ea574bd2e69b65cc5772c341de653a#diff-bbce44daacb259a7b129c4fa0d062406

To be clear, I am using @powerbuoy's test case, but am seeing this behavior all over my app.

StrahilKazlachev commented 6 years ago

Here is how I reproduced it:

  1. Create a new webpack/requirejs project using aurelia-cli.
  2. Copy&Paste the app.js/html from the first post.

What I saw:

  1. Creating new array, lets say like this this.items = [...this.items, [{}]];, instead of mutating the current one makes it work as expected.
  2. When using .push the ChangeRecords seem correct to me. Here is one for the 3rd addition:
    [{"index":2,"removed":[],"addedCount":1}]
  3. If the element with the repeat is not a descendant of an element with an if, all works as expected.
  4. I see the same behavior with aurelia-templating-resources@1.4.0.
jods4 commented 6 years ago

I see the same behavior with aurelia-templating-resources@1.4.0.

Quite important point to notice because the timing is unlucky. 1.5 contained changes in if with the introduction of else and a few important bugs that got fixed in 1.5.4.

If it reproduces with templating-resources@1.4.0 it means the bug is not in the recent if changes but was there before.

bigopon commented 6 years ago

The issue is because instanceChanged and instanceMutated happen at the same time by this.items.push({}) causing repeat to add duplicated view. Temporary workaround is to make sure they don't happen in the same micro task flush via:

  // Toggle the edit form
  toggleEditing () {
    this.editing = !this.editing;

    // Also add a new item
    if (this.editing) {
      setTimeout(() => this.items.push({}));
    }
  }

I'm not sure how to fix this yet 😅

jods4 commented 6 years ago

A little out of the blue because I haven't looked at the code in depth but maybe: instanceMutated should look at what the current instance in the repeat viewmodel is. If it's not the one being mutated, there is a race condition in the notification microtasks and the mutation should be ignored.

EDIT: although that probably wouldn't handle ABA cases, but maybe that's a little far fetched.

pkkummermo commented 6 years ago

I've had the same issue, but not in 1.4.0 of templating-resources (see https://github.com/aurelia/templating/issues/580 for more details)

cbrownie commented 6 years ago

My team has also encountered the issue that @BlueManiac has outlined exactly. We were battling with it for awhile, and ending up reverting our package.json to an earlier working state from about a month ago to get around it. Would be very interested in a fix.

StrahilKazlachev commented 6 years ago

@cbrownie Could you post the versions of aurelia-templating and aurelia-templating-resources with which you don't get this issue?

cbrownie commented 6 years ago

@StrahilKazlachev These are the aurelia package versions we were using that produced the issue with repeaters and other odd errors.

"aurelia-animator-css": "^1.0.4",
"aurelia-api": "^3.1.1",
"aurelia-bootstrapper": "^2.1.1",
"aurelia-dependency-injection": "^1.3.2",
"aurelia-dialog": "^1.0.0-rc.1.0.3",
"aurelia-event-aggregator": "^1.0.1",
"aurelia-fetch-client": "^1.1.3",
"aurelia-framework": "^1.1.5",
"aurelia-history-browser": "^1.1.0",
"aurelia-http-client": "^1.2.1",
"aurelia-i18n": "^1.6.2",
"aurelia-loader-default": "^1.0.3",
"aurelia-logging": "^1.3.1",
"aurelia-logging-console": "^1.0.0",
"aurelia-pal": "^1.4.0",
"aurelia-pal-browser": "^1.3.0",
"aurelia-path": "^1.1.1",
"aurelia-permission": "^0.5.3",
"aurelia-polyfills": "^1.2.2",
"aurelia-router": "^1.4.0",
"aurelia-templating-binding": "^1.4.0",
"aurelia-templating-resources": "1.5.3",
"aurelia-templating-router": "^1.2.0",
"aurelia-validation": "^1.1.2",

This is a seemingly stable excerpt of our package.json from a couple months ago.

"aurelia-animator-css": "^1.0.0",
"aurelia-api": "^3.1.1",
"aurelia-bootstrapper": "^2.1.1",
"aurelia-dependency-injection": "^1.0.0",
"aurelia-dialog": "^1.0.0-rc.1.0.3",
"aurelia-event-aggregator": "^1.0.0",
"aurelia-fetch-client": "^1.0.0",
"aurelia-framework": "^1.0.0",
"aurelia-history-browser": "^1.0.0",
"aurelia-http-client": "^1.0.0",
"aurelia-i18n": "^1.1.2",
"aurelia-loader-default": "^1.0.0",
"aurelia-logging": "^1.0.0",
"aurelia-logging-console": "^1.0.0",
"aurelia-pal": "^1.0.0",
"aurelia-pal-browser": "^1.0.0",
"aurelia-path": "^1.0.0",
"aurelia-permission": "^0.5.2",
"aurelia-polyfills": "^1.0.0",
"aurelia-router": "^1.0.0",
"aurelia-templating-binding": "^1.0.0",
"aurelia-templating-resources": "^1.0.0",
"aurelia-templating-router": "^1.0.0",

I can't figure out exactly why this revert is fixing the issue because our use of ^ in both instances should be grabbing the same version of aurelia-templating, and aurelia-templating-resources.

On a second look the only package that really changed a major version in our package.json was webpack: "webpack": "^3.8.1" --(back to)--> "webpack": "^2.6.1".

We also upgraded npm itself from a 3.x.x to a 5.x.x version on our Jenkins build server that handled running npm install.

StrahilKazlachev commented 6 years ago

@cbrownie Thanks, that is really strange. I really wanted to easily pinpoint when the problem was introduced 😅. Also Webpack should not be an issue since I reproduced it with RequireJS.

phanithinks commented 6 years ago

Any update on this. I am trying to call a function in repeat.for. its been called twice, I cant able to replicate it in gist . One interesting thing i observed. when we set break-point on called function. it works fine.

I am using aurelia-templating-resources@1.5.4

pkkummermo commented 6 years ago

I've had to temporary revert the version due to the numerous places it breaks within our application. Hopefully this issue will be fixed and I can update to the newest :+1:

Just to clarify what I'm reverting:

-        "aurelia-templating": "1.6.0",
-        "aurelia-templating-binding": "1.4.0",
-        "aurelia-templating-resources": "1.5.4",
+        "aurelia-templating": "1.4.2",
+        "aurelia-templating-binding": "1.3.0",
+        "aurelia-templating-resources": "1.4.0",

After this it works perfectly again.

Alexander-Taran commented 6 years ago

@pkkummermo can you check out if it is resolved with latest versions?

pkkummermo commented 6 years ago

Sure, I'll do an upgrade test today :)

royston-c commented 6 years ago

Hi. I am not sure if i am experiencing exactly the same issue detailed in this thread. However, I have an issue with the repeat.for attribute rendering twice in an Aurelia App/Widget (creating via the bootstrap((aurelia: Aurelia) => { //.... })) inside another Aurelia app.

It's almost as if Aurelia is proxying the array functions (push, splice etc) twice. Although it's worth noting that the unexpected HTML elements have no view model (aka they render no values to the DOM). Just like @BlueManiac mentioned in his post dated 13 Nov 2017.

If I create a new array instead of using push, splice, unshift everything works fine. Example that works as expected:

export class BugFixForRepeatingArrayValueConverter {
    public toView(array: any[]) {
        return [].concat(array);
    }
}

<div repeat.for="i of myArray | bugFixForRepeatingArray">
    ${ i }              
</div>

using:


aurelia-binding: 1.7.1
aurelia-templating: 1.7.0 
aurelia-templating-binding: 1.4.1
aurelia-templating-resources: 1.6.0```

Note: if this widget is not hosted in another Aurelia app everything works as expected.
Alexander-Taran commented 6 years ago

@roy46 could you describe a bit more your double aurelia setup?

royston-c commented 6 years ago

I have a widget class that exposes an init() to start up the component:

import { PLATFORM } from "aurelia-pal";
import { bootstrap } from "aurelia-bootstrapper";
import { Aurelia } from "aurelia-framework";

export default class MyWidget {
    constructor(private config: WidgetConfig) {
    }

    public init() {
        bootstrap((aurelia: Aurelia) => {
            aurelia.use
                .standardConfiguration()
                .developmentLogging();

            aurelia.start().then(() => {
                aurelia.setRoot(PLATFORM.moduleName("widget/app-widget"), this.config.element);
            });
        });
    }
}

Then I have created a test harness (another Aurelia app) that imports and uses that widget. In the test harness I created a wrapper component:

export class WidgetWrapper {

    @bindable public property1: string;
    @bindable public property2: string;
    private _widget: MyWidget ;

    constructor(private element: Element) { }

    public attached() {
        this._widget = new MyWidget({
            element: <HTMLElement>this.element,
            property1: this.property1,
            property2: this.property2
        });

        this._widget.init();
    }
}
Alexander-Taran commented 6 years ago

@roy46 can't explain why, but it looks like a way to disaster (-:

pkkummermo commented 6 years ago

@Alexander-Taran I tried updating to the newest versions, ie

    "aurelia-templating": "^1.7.0",
    "aurelia-templating-binding": "1.4.1",
    "aurelia-templating-resources": "1.6.0",

and I'm afraid the problem still persists. Is there any deps I should be updating alongside these which might cause interference?

Alexander-Taran commented 6 years ago

Ok then. Got nothing to add for the moment.

pkkummermo commented 6 years ago

Cool, let me know if you want me to test anything else.

PS: Could aurelia-dialog interfere here?

I'm using:

"aurelia-dialog": "1.0.0-rc.1.0.3",

I see in the diff to master now they have explicit versions in their deps. I can try to bump versions and give feedback.

pkkummermo commented 6 years ago

OK. Went through several steps now.

  1. Went to the CLI to see if I was missing any dependencies. Added several (which I guess was mostly implicit within the other existing deps).
  2. Updated all aurelia-related packages with the exception of the aurelia-webpack-plugin (haven't updated to WP4 yet).
  3. Still no go.

Example of a simple dialog with two pushes over time: image (# is #${ $index }) Funny thing is that it correctly shows a normal array and then just doubles up with empty items.

I would have expected something like this:

ITEM1
<empty item>
ITEM2
<empty item>

But it shows as

ITEM 1
ITEM 2
<empty item>
<empty item>

Let me know if you want me to debug anything deeper @Alexander-Taran

Alexander-Taran commented 6 years ago

@pkkummermo will do.. maybe.. if.. and when (-:

amartens181 commented 6 years ago

In some components, if I put the function to set the array in the bind method, it will duplicate the first half items. It works correctly in the attached method.

mikeesouth commented 6 years ago

Any updates here? I got the same issue. I have a repeater like so:

<div repeat.for="item of myArray">
  ${item.name}
</div>

I subscribe to the EventAggregator and when an updated array is sent in an event I update myArray like this:

this.myArray.splice(0, this.myArray.length);
updatedArray.forEach(c => this.myArray.push(c));

I specifically did this to not reassign the array but rather clear it and update it. This was to prevent the binding from "being lost" in earlier versions. This causes empty items in my repeater, just like @pkkummermo describes (ITEM1, ITEM2, EMPTY, EMPTY). I can fix this by replacing my splice + push logic to a reassign, like this: this.myArray = [...updatedArray];

Then it works. But I have to this in several places in my code and it's quite difficult to find them.

Any news on this issue? Will it be fixed or should I just start to convert to reassigning my arrays that I use for repeaters?

EisenbergEffect commented 6 years ago

@bigopon I think this would be a great issue for you to look into :)