From 92e4b7f813bf08efad4b04e7c8c1c4b6b9cd0a57 Mon Sep 17 00:00:00 2001 From: Rich-Harris <richard.a.harris@gmail.com> Date: Sat, 1 Apr 2017 17:10:08 -0400 Subject: [PATCH 1/2] prevent hard-to-reproduce bug with deep two-way bindings --- .../attributes/addComponentBinding.js | 4 +- .../visitors/attributes/addElementBinding.js | 2 +- .../visitors/attributes/binding/getSetter.js | 4 +- .../ComponentSelector.html | 5 ++ .../component-binding-deep-b/Editor.html | 1 + .../component-binding-deep-b/_config.js | 84 +++++++++++++++++++ .../component-binding-deep-b/main.html | 42 ++++++++++ 7 files changed, 137 insertions(+), 5 deletions(-) create mode 100644 test/generator/samples/component-binding-deep-b/ComponentSelector.html create mode 100644 test/generator/samples/component-binding-deep-b/Editor.html create mode 100644 test/generator/samples/component-binding-deep-b/_config.js create mode 100644 test/generator/samples/component-binding-deep-b/main.html diff --git a/src/generators/dom/visitors/attributes/addComponentBinding.js b/src/generators/dom/visitors/attributes/addComponentBinding.js index 5b740aa29fd9..07ae7fefe811 100644 --- a/src/generators/dom/visitors/attributes/addComponentBinding.js +++ b/src/generators/dom/visitors/attributes/addComponentBinding.js @@ -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!' ); @@ -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; diff --git a/src/generators/dom/visitors/attributes/addElementBinding.js b/src/generators/dom/visitors/attributes/addElementBinding.js index ee222dc304ba..a13d254dfbea 100644 --- a/src/generators/dom/visitors/attributes/addElementBinding.js +++ b/src/generators/dom/visitors/attributes/addElementBinding.js @@ -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 diff --git a/src/generators/dom/visitors/attributes/binding/getSetter.js b/src/generators/dom/visitors/attributes/binding/getSetter.js index a63c54304201..c6bf99c0c229 100644 --- a/src/generators/dom/visitors/attributes/binding/getSetter.js +++ b/src/generators/dom/visitors/attributes/binding/getSetter.js @@ -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 ) : ''; @@ -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} }); `; } diff --git a/test/generator/samples/component-binding-deep-b/ComponentSelector.html b/test/generator/samples/component-binding-deep-b/ComponentSelector.html new file mode 100644 index 000000000000..42cc676039b7 --- /dev/null +++ b/test/generator/samples/component-binding-deep-b/ComponentSelector.html @@ -0,0 +1,5 @@ +<select bind:value='selectedComponent'> + {{#each components as component}} + <option value='{{component}}'>{{component.name}}.html</option> + {{/each}} +</select> \ No newline at end of file diff --git a/test/generator/samples/component-binding-deep-b/Editor.html b/test/generator/samples/component-binding-deep-b/Editor.html new file mode 100644 index 000000000000..34ed70114bc7 --- /dev/null +++ b/test/generator/samples/component-binding-deep-b/Editor.html @@ -0,0 +1 @@ +<textarea bind:value='code'></textarea> \ No newline at end of file diff --git a/test/generator/samples/component-binding-deep-b/_config.js b/test/generator/samples/component-binding-deep-b/_config.js new file mode 100644 index 000000000000..b058bdeb43ae --- /dev/null +++ b/test/generator/samples/component-binding-deep-b/_config.js @@ -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(); + } +}; diff --git a/test/generator/samples/component-binding-deep-b/main.html b/test/generator/samples/component-binding-deep-b/main.html new file mode 100644 index 000000000000..46cdd2f6d0bf --- /dev/null +++ b/test/generator/samples/component-binding-deep-b/main.html @@ -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> \ No newline at end of file From cf626ff880ae077bf84fd7cce2f6b1855fa0012c Mon Sep 17 00:00:00 2001 From: Rich-Harris <richard.a.harris@gmail.com> Date: Sat, 1 Apr 2017 17:19:11 -0400 Subject: [PATCH 2/2] retain binding sourcemaps to the extent possible --- src/utils/flattenReference.js | 5 ++++- test/sourcemaps/samples/binding/test.js | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/utils/flattenReference.js b/src/utils/flattenReference.js index da92d5588f37..c4b46e914f81 100644 --- a/src/utils/flattenReference.js +++ b/src/utils/flattenReference.js @@ -1,5 +1,7 @@ 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 ); @@ -7,10 +9,11 @@ export default function flatten ( node ) { 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}✂]` }; } diff --git a/test/sourcemaps/samples/binding/test.js b/test/sourcemaps/samples/binding/test.js index a8803c00631d..8737930371b1 100644 --- a/test/sourcemaps/samples/binding/test.js +++ b/test/sourcemaps/samples/binding/test.js @@ -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, @@ -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,