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

prevent hard-to-reproduce bug with deep two-way bindings #432

Merged
merged 2 commits into from
Apr 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/generators/dom/visitors/attributes/addComponentBinding.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import flattenReference from '../../../../utils/flattenReference.js';
import getSetter from './binding/getSetter.js';

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

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

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

generator.hasComplexBindings = true;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default function createBinding ( generator, node, attribute, current, loc
const value = getBindingValue( generator, local, node, attribute, isMultipleSelect, bindingGroup, type );
const eventName = getBindingEventName( node );

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

// <select> special case
Expand Down
4 changes: 2 additions & 2 deletions src/generators/dom/visitors/attributes/binding/getSetter.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import deindent from '../../../../../utils/deindent.js';

export default function getSetter ({ current, name, context, attribute, dependencies, snippet, value }) {
export default function getSetter ({ current, name, keypath, context, attribute, dependencies, value }) {
if ( current.contexts.has( name ) ) {
const prop = dependencies[0];
const tail = attribute.value.type === 'MemberExpression' ? getTailSnippet( attribute.value ) : '';
Expand All @@ -17,7 +17,7 @@ export default function getSetter ({ current, name, context, attribute, dependen
if ( attribute.value.type === 'MemberExpression' ) {
return deindent`
var ${name} = ${current.component}.get( '${name}' );
${snippet} = ${value};
${keypath} = ${value};
${current.component}._set({ ${name}: ${name} });
`;
}
Expand Down
5 changes: 4 additions & 1 deletion src/utils/flattenReference.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
export default function flatten ( node ) {
const parts = [];
const propEnd = node.end;

while ( node.type === 'MemberExpression' ) {
if ( node.computed ) return null;
parts.unshift( node.property.name );

node = node.object;
}

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

if ( !name ) return null;

parts.unshift( name );
return { name, parts, keypath: parts.join( '.' ) };
return { name, parts, keypath: `${name}[✂${propStart}-${propEnd}✂]` };
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<select bind:value='selectedComponent'>
{{#each components as component}}
<option value='{{component}}'>{{component.name}}.html</option>
{{/each}}
</select>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<textarea bind:value='code'></textarea>
84 changes: 84 additions & 0 deletions test/generator/samples/component-binding-deep-b/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
const components = [
{
name: 'One',
source: 'one source'
},
{
name: 'Two',
source: 'two source'
}
];

const selectedComponent = components[0];

export default {
skip: true, // doesn't reflect real-world bug, maybe a JSDOM quirk

data: {
components,
selectedComponent
},

html: `
<select>
<option value='[object Object]'>One.html</option>
<option value='[object Object]'>Two.html</option>
</select>

<textarea></textarea>

<pre>ONE SOURCE\nTWO SOURCE</pre>
`,

test ( assert, component, target, window ) {
const event = new window.MouseEvent( 'input' );
const textarea = target.querySelector( 'textarea' );

textarea.value = 'one source changed';
textarea.dispatchEvent( event );

assert.equal( component.get( 'compiled' ), 'ONE SOURCE CHANGED\nTWO SOURCE' );
assert.htmlEqual( target.innerHTML, `
<select>
<option value='[object Object]'>One.html</option>
<option value='[object Object]'>Two.html</option>
</select>

<textarea></textarea>

<pre>ONE SOURCE CHANGED\nTWO SOURCE</pre>
` );

// const select = target.querySelector( 'select' );
// console.log( `select.options[0].selected`, select.options[0].selected )
// console.log( `select.options[1].selected`, select.options[1].selected )
// console.log( `select.value`, select.value )
// console.log( `select.__value`, select.__value )
// select.options[1].selected = true;
// console.log( `select.options[0].selected`, select.options[0].selected )
// console.log( `select.options[1].selected`, select.options[1].selected )
// console.log( `select.value`, select.value )
// console.log( `select.__value`, select.__value )
// select.dispatchEvent( new window.Event( 'change' ) );
component.set({ selectedComponent: components[1] });

assert.equal( textarea.value, 'two source' );

textarea.value = 'two source changed';
textarea.dispatchEvent( event );

assert.equal( component.get( 'compiled' ), 'ONE SOURCE CHANGED\nTWO SOURCE CHANGED' );
assert.htmlEqual( target.innerHTML, `
<select>
<option value='[object Object]'>One.html</option>
<option value='[object Object]'>Two.html</option>
</select>

<textarea></textarea>

<pre>ONE SOURCE CHANGED\nTWO SOURCE CHANGED</pre>
` );

component.destroy();
}
};
42 changes: 42 additions & 0 deletions test/generator/samples/component-binding-deep-b/main.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<ComponentSelector :components bind:selectedComponent/>
<Editor bind:code='selectedComponent.source'/>

<pre>
{{compiled}}
</pre>

<script>
import Editor from './Editor.html';
import ComponentSelector from './ComponentSelector.html';

export default {
components: {
ComponentSelector,
Editor
},

oncreate () {
this.observe( 'components', components => {
components.forEach( component => {
if ( component === this.get( 'selectedComponent' ) ) return;
component.compiled = component.source.toUpperCase();
});
});

this.observe( 'selectedComponent', component => {
component.compiled = component.source.toUpperCase();
this.updateBundle();
});
},

methods: {
updateBundle () {
const components = this.get( 'components' );

const compiled = components.map( component => component.compiled ).join( '\n' );

this.set({ compiled });
}
}
}
</script>
6 changes: 3 additions & 3 deletions test/sourcemaps/samples/binding/test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
export function test ({ assert, smc, locateInSource, locateInGenerated }) {
const expected = locateInSource( 'foo.bar.baz' );
const expected = locateInSource( 'bar.baz' );

let loc;
let actual;

loc = locateInGenerated( 'foo.bar.baz' );
loc = locateInGenerated( 'bar.baz' );

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

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

actual = smc.originalPositionFor({
line: loc.line + 1,
Expand Down