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

preact@10.22.0 unexpected child ordering with skew based diff #4412

Closed
1 task done
hzy opened this issue Jun 18, 2024 · 2 comments · Fixed by #4413
Closed
1 task done

preact@10.22.0 unexpected child ordering with skew based diff #4412

hzy opened this issue Jun 18, 2024 · 2 comments · Fixed by #4413

Comments

@hzy
Copy link

hzy commented Jun 18, 2024

  • Check if updating to the latest Preact version resolves the issue (Checked, the issue still reproduce)

Describe the bug
With preact version from 10.16.0 to 10.22.0, and code below:

<div>
  {items.map((key) => (
    <div key={key}>{key}</div>
  ))}
</div>

Updating array items (shuffle it) leads to unexpected ordering.

preact version 10.15.0 and below will not reproduce this issue.

To Reproduce (with Stackblitz)

Steps to reproduce the behavior:

  1. Open project at https://stackblitz.com/edit/create-preact-starter-ckjsze?file=src%2Findex.jsx
  2. You can see numbers in order:
    1
    3
    4
    0
    5
    2
    6
    

Expected behavior
Numbers should be in order:

1
3
5
2
6
4
0

To Reproduce (in preact unit test)

  1. Put code below inside file test/browser/render.test.js,
    it("shuffle", function () {
        const jsx = (items) => (
    	    <div>
    		    {items.map((key) => (
    			    <div key={key}>{key}</div>
    		    ))}
    	    </div>
        );
        const a = ["0", "1", "2", "3", "4", "5", "6"];
        const b = ["1", "3", "5", "2", "6", "4", "0"];
        render(jsx(a), scratch);
        render(jsx(b), scratch);
    
        expect(scratch.innerHTML).to.equal(
    	    `<div>${b.map(n => `<div>${n}</div>`).join("")}</div>`
        );
    });
  2. run test, the test above will FAIL

Expected behavior
Test should pass.

@hzy
Copy link
Author

hzy commented Aug 9, 2024

@JoviDeCroock Thank you for your timely fix.
But I think this problem has not been completely solved, if runs on highly randomized input.

To Reproduce (in preact unit test)

  1. Put code below inside file test/browser/render.test.js,
it('handle shuffled', () => {
	function randomize(arr) {
		for (let i = arr.length - 1; i > 0; i--) {
			let j = Math.floor(Math.random() * (i + 1));
			[arr[i], arr[j]] = [arr[j], arr[i]];
		}
		return arr;
	}

	const App = ({items}) => (
		<div>
			{items.map(key => (
			<div key={key}>
				{key}
			</div>
			))}
		</div>
	);

	const a = Array.from({ length: 8 }).map((_, i) => `${i}`);

	for (let i = 0; i < 10000; i++) {
		const aa = randomize(a);
		render(<App items={aa}/>, scratch);
		expect(scratch.innerHTML).to.equal(
			`<div>${aa.map(n => `<div>${n}</div>`).join('')}</div>`
		);
	}
});
  1. run test, the test above will FAIL

Expected behavior
Test should pass.

@JoviDeCroock
Copy link
Member

@hzy Thank you so much for reporting these, added a log here

	for (let i = 0; i < 10000; i++) {
		const aa = randomize(a);
                console.log(aa)
		render(<App items={aa}/>, scratch);
		expect(scratch.innerHTML).to.equal(
			`<div>${aa.map(n => `<div>${n}</div>`).join('')}</div>`
		);
	}

To find the exact sequence that makes it fail and made #4472 to fix it. Thank you for bearing with me to find this fix. In the future you would be better off creating a new issue, I came across this by accident 😅 closed issues are often unsubscribed from

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 a pull request may close this issue.

2 participants