Skip to content

Commit 7c5eaa2

Browse files
Rich-HarrisRich-Harris
authored
Merge pull request #432 from sveltejs/binding-mutation-bug
prevent hard-to-reproduce bug with deep two-way bindings
2 parents 38ee4f1 + cf626ff commit 7c5eaa2

File tree

9 files changed

+144
-9
lines changed

9 files changed

+144
-9
lines changed

‎src/generators/dom/visitors/attributes/addComponentBinding.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import flattenReference from '../../../../utils/flattenReference.js';
33
import getSetter from './binding/getSetter.js';
44

55
export default function createBinding ( generator, node, attribute, current, local ) {
6-
const { name } = flattenReference( attribute.value );
6+
const { name, keypath } = flattenReference( attribute.value );
77
const { snippet, contexts, dependencies } = generator.contextualise( attribute.value );
88

99
if ( dependencies.length > 1 ) throw new Error( 'An unexpected situation arose. Please raise an issue at https://github.com/sveltejs/svelte/issues — thanks!' );
@@ -35,7 +35,7 @@ export default function createBinding ( generator, node, attribute, current, loc
3535
prop
3636
});
3737

38-
const setter = getSetter({ current, name, context: '_context', attribute, dependencies, snippet, value: 'value' });
38+
const setter = getSetter({ current, name, keypath, context: '_context', attribute, dependencies, value: 'value' });
3939

4040
generator.hasComplexBindings = true;
4141

‎src/generators/dom/visitors/attributes/addElementBinding.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export default function createBinding ( generator, node, attribute, current, loc
2121
const value = getBindingValue( generator, local, node, attribute, isMultipleSelect, bindingGroup, type );
2222
const eventName = getBindingEventName( node );
2323

24-
let setter = getSetter({ current, name, context: '__svelte', attribute, dependencies, snippet, value });
24+
let setter = getSetter({ current, name, keypath, context: '__svelte', attribute, dependencies, value });
2525
let updateElement;
2626

2727
// <select> special case

‎src/generators/dom/visitors/attributes/binding/getSetter.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import deindent from '../../../../../utils/deindent.js';
22

3-
export default function getSetter ({ current, name, context, attribute, dependencies, snippet, value }) {
3+
export default function getSetter ({ current, name, keypath, context, attribute, dependencies, value }) {
44
if ( current.contexts.has( name ) ) {
55
const prop = dependencies[0];
66
const tail = attribute.value.type === 'MemberExpression' ? getTailSnippet( attribute.value ) : '';
@@ -17,7 +17,7 @@ export default function getSetter ({ current, name, context, attribute, dependen
1717
if ( attribute.value.type === 'MemberExpression' ) {
1818
return deindent`
1919
var ${name} = ${current.component}.get( '${name}' );
20-
${snippet} = ${value};
20+
${keypath} = ${value};
2121
${current.component}._set({ ${name}: ${name} });
2222
`;
2323
}

‎src/utils/flattenReference.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,19 @@
11
export default function flatten ( node ) {
22
const parts = [];
3+
const propEnd = node.end;
4+
35
while ( node.type === 'MemberExpression' ) {
46
if ( node.computed ) return null;
57
parts.unshift( node.property.name );
68

79
node = node.object;
810
}
911

12+
const propStart = node.end;
1013
const name = node.type === 'Identifier' ? node.name : node.type === 'ThisExpression' ? 'this' : null;
1114

1215
if ( !name ) return null;
1316

1417
parts.unshift( name );
15-
return { name, parts, keypath: parts.join( '.' ) };
18+
return { name, parts, keypath: `${name}[✂${propStart}-${propEnd}✂]` };
1619
}

‎test/generator/samples/component-binding-deep-b/ComponentSelector.html

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<select bind:value='selectedComponent'>
2+
{{#each components as component}}
3+
<option value='{{component}}'>{{component.name}}.html</option>
4+
{{/each}}
5+
</select>

‎test/generator/samples/component-binding-deep-b/Editor.html

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<textarea bind:value='code'></textarea>

‎test/generator/samples/component-binding-deep-b/_config.js

+84
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
const components = [
2+
{
3+
name: 'One',
4+
source: 'one source'
5+
},
6+
{
7+
name: 'Two',
8+
source: 'two source'
9+
}
10+
];
11+
12+
const selectedComponent = components[0];
13+
14+
export default {
15+
skip: true, // doesn't reflect real-world bug, maybe a JSDOM quirk
16+
17+
data: {
18+
components,
19+
selectedComponent
20+
},
21+
22+
html: `
23+
<select>
24+
<option value='[object Object]'>One.html</option>
25+
<option value='[object Object]'>Two.html</option>
26+
</select>
27+
28+
<textarea></textarea>
29+
30+
<pre>ONE SOURCE\nTWO SOURCE</pre>
31+
`,
32+
33+
test ( assert, component, target, window ) {
34+
const event = new window.MouseEvent( 'input' );
35+
const textarea = target.querySelector( 'textarea' );
36+
37+
textarea.value = 'one source changed';
38+
textarea.dispatchEvent( event );
39+
40+
assert.equal( component.get( 'compiled' ), 'ONE SOURCE CHANGED\nTWO SOURCE' );
41+
assert.htmlEqual( target.innerHTML, `
42+
<select>
43+
<option value='[object Object]'>One.html</option>
44+
<option value='[object Object]'>Two.html</option>
45+
</select>
46+
47+
<textarea></textarea>
48+
49+
<pre>ONE SOURCE CHANGED\nTWO SOURCE</pre>
50+
` );
51+
52+
// const select = target.querySelector( 'select' );
53+
// console.log( `select.options[0].selected`, select.options[0].selected )
54+
// console.log( `select.options[1].selected`, select.options[1].selected )
55+
// console.log( `select.value`, select.value )
56+
// console.log( `select.__value`, select.__value )
57+
// select.options[1].selected = true;
58+
// console.log( `select.options[0].selected`, select.options[0].selected )
59+
// console.log( `select.options[1].selected`, select.options[1].selected )
60+
// console.log( `select.value`, select.value )
61+
// console.log( `select.__value`, select.__value )
62+
// select.dispatchEvent( new window.Event( 'change' ) );
63+
component.set({ selectedComponent: components[1] });
64+
65+
assert.equal( textarea.value, 'two source' );
66+
67+
textarea.value = 'two source changed';
68+
textarea.dispatchEvent( event );
69+
70+
assert.equal( component.get( 'compiled' ), 'ONE SOURCE CHANGED\nTWO SOURCE CHANGED' );
71+
assert.htmlEqual( target.innerHTML, `
72+
<select>
73+
<option value='[object Object]'>One.html</option>
74+
<option value='[object Object]'>Two.html</option>
75+
</select>
76+
77+
<textarea></textarea>
78+
79+
<pre>ONE SOURCE CHANGED\nTWO SOURCE CHANGED</pre>
80+
` );
81+
82+
component.destroy();
83+
}
84+
};

‎test/generator/samples/component-binding-deep-b/main.html

+42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<ComponentSelector :components bind:selectedComponent/>
2+
<Editor bind:code='selectedComponent.source'/>
3+
4+
<pre>
5+
{{compiled}}
6+
</pre>
7+
8+
<script>
9+
import Editor from './Editor.html';
10+
import ComponentSelector from './ComponentSelector.html';
11+
12+
export default {
13+
components: {
14+
ComponentSelector,
15+
Editor
16+
},
17+
18+
oncreate () {
19+
this.observe( 'components', components => {
20+
components.forEach( component => {
21+
if ( component === this.get( 'selectedComponent' ) ) return;
22+
component.compiled = component.source.toUpperCase();
23+
});
24+
});
25+
26+
this.observe( 'selectedComponent', component => {
27+
component.compiled = component.source.toUpperCase();
28+
this.updateBundle();
29+
});
30+
},
31+
32+
methods: {
33+
updateBundle () {
34+
const components = this.get( 'components' );
35+
36+
const compiled = components.map( component => component.compiled ).join( '\n' );
37+
38+
this.set({ compiled });
39+
}
40+
}
41+
}
42+
</script>

‎test/sourcemaps/samples/binding/test.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
export function test ({ assert, smc, locateInSource, locateInGenerated }) {
2-
const expected = locateInSource( 'foo.bar.baz' );
2+
const expected = locateInSource( 'bar.baz' );
33

44
let loc;
55
let actual;
66

7-
loc = locateInGenerated( 'foo.bar.baz' );
7+
loc = locateInGenerated( 'bar.baz' );
88

99
actual = smc.originalPositionFor({
1010
line: loc.line + 1,
@@ -18,7 +18,7 @@ export function test ({ assert, smc, locateInSource, locateInGenerated }) {
1818
column: expected.column
1919
});
2020

21-
loc = locateInGenerated( 'foo.bar.baz', loc.character + 1 );
21+
loc = locateInGenerated( 'bar.baz', loc.character + 1 );
2222

2323
actual = smc.originalPositionFor({
2424
line: loc.line + 1,

0 commit comments

Comments
 (0)