From 41724067bf47bba04ee89dca11f05313371a9ffb Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Sat, 11 May 2019 11:29:11 -0300 Subject: [PATCH] FIX #2446: apply bindings and event handlers in order --- .../render-dom/wrappers/Element/index.ts | 133 ++++++++++++------ test/js/samples/media-bindings/expected.js | 40 +++--- .../apply-directives-in-order/_config.js | 37 +++++ .../apply-directives-in-order/main.svelte | 10 ++ 4 files changed, 156 insertions(+), 64 deletions(-) create mode 100644 test/runtime/samples/apply-directives-in-order/_config.js create mode 100644 test/runtime/samples/apply-directives-in-order/main.svelte diff --git a/src/compile/render-dom/wrappers/Element/index.ts b/src/compile/render-dom/wrappers/Element/index.ts index 22ea7a78cd38..d23aa43f192d 100644 --- a/src/compile/render-dom/wrappers/Element/index.ts +++ b/src/compile/render-dom/wrappers/Element/index.ts @@ -21,6 +21,8 @@ import add_actions from '../shared/add_actions'; import create_debugging_comment from '../shared/create_debugging_comment'; import { get_context_merger } from '../shared/get_context_merger'; import Slot from '../../../nodes/Slot'; +import EventHandler from "../../../nodes/EventHandler"; +import BindingWrapper from "./Binding"; const events = [ { @@ -308,8 +310,7 @@ export default class ElementWrapper extends Wrapper { block.maintain_context = true; } - this.add_bindings(block); - this.add_event_handlers(block); + this.add_directives_in_order(block); this.add_attributes(block); this.add_transitions(block); this.add_animation(block); @@ -389,29 +390,60 @@ export default class ElementWrapper extends Wrapper { : `{}`}, ${this.node.namespace === namespaces.svg ? true : false})`; } - add_bindings(block: Block) { + add_directives_in_order (block: Block) { + const bindingGroups = events + .map(event => ({ + events: event.event_names, + bindings: this.bindings + .filter(binding => binding.node.name !== 'this') + .filter(binding => event.filter(this.node, binding.node.name)) + })) + .filter(group => group.bindings.length); + + const this_binding = this.bindings.find(b => b.node.name === 'this'); + + function getOrder (item) { + if (item instanceof EventHandler) { + return (item as EventHandler).start; + } else if (item instanceof BindingWrapper) { + return (item as BindingWrapper).node.start + } else { + return item.bindings[0].node.start + } + } + + const ordered = [ + ...bindingGroups, + ...this.node.handlers, + ...(this_binding ? [this_binding] : []) + ]; + ordered.sort((a, b) => getOrder(a) - getOrder(b)); + + ordered.forEach(bindingGroupOrEventHandler => { + if (bindingGroupOrEventHandler instanceof EventHandler) { + add_event_handlers(block, this.var, [bindingGroupOrEventHandler as EventHandler]) + } else if (bindingGroupOrEventHandler instanceof BindingWrapper) { + this.add_this_binding(block, bindingGroupOrEventHandler as BindingWrapper); + } else { + this.add_binding(block, bindingGroupOrEventHandler); + } + }); + } + + add_binding(block: Block, bindingGroup) { const { renderer } = this; - if (this.bindings.length === 0) return; + if (bindingGroup.bindings.length === 0) return; renderer.component.has_reactive_assignments = true; - const lock = this.bindings.some(binding => binding.needs_lock) ? + const lock = bindingGroup.bindings.some(binding => binding.needs_lock) ? block.get_unique_name(`${this.var}_updating`) : null; if (lock) block.add_variable(lock, 'false'); - const groups = events - .map(event => ({ - events: event.event_names, - bindings: this.bindings - .filter(binding => binding.node.name !== 'this') - .filter(binding => event.filter(this.node, binding.node.name)) - })) - .filter(group => group.bindings.length); - - groups.forEach(group => { + [bindingGroup].forEach(group => { const handler = renderer.component.get_unique_name(`${this.var}_${group.events.join('_')}_handler`); renderer.component.add_var({ @@ -514,42 +546,55 @@ export default class ElementWrapper extends Wrapper { if (lock) { block.builders.update.add_line(`${lock} = false;`); } + } - const this_binding = this.bindings.find(b => b.node.name === 'this'); - if (this_binding) { - const name = renderer.component.get_unique_name(`${this.var}_binding`); + add_this_binding(block: Block, this_binding: BindingWrapper) { + const { renderer } = this; - renderer.component.add_var({ - name, - internal: true, - referenced: true - }); + renderer.component.has_reactive_assignments = true; - const { handler, object } = this_binding; + const lock = this.bindings.some(binding => binding.needs_lock) ? + block.get_unique_name(`${this.var}_updating`) : + null; - const args = []; - for (const arg of handler.contextual_dependencies) { - args.push(arg); - block.add_variable(arg, `ctx.${arg}`); - } + if (lock) block.add_variable(lock, 'false'); - renderer.component.partly_hoisted.push(deindent` - function ${name}(${['$$node', 'check'].concat(args).join(', ')}) { - ${handler.snippet ? `if ($$node || (!$$node && ${handler.snippet} === check)) ` : ''}${handler.mutation} - ${renderer.component.invalidate(object)}; - } - `); + if (lock) { + block.builders.update.add_line(`${lock} = false;`); + } - block.builders.mount.add_line(`@add_binding_callback(() => ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}));`); - block.builders.destroy.add_line(`ctx.${name}(${['null', this.var].concat(args).join(', ')});`); - block.builders.update.add_line(deindent` - if (changed.items) { - ctx.${name}(${['null', this.var].concat(args).join(', ')}); - ${args.map(a => `${a} = ctx.${a}`).join(', ')}; - ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}); - }` - ); + const name = renderer.component.get_unique_name(`${this.var}_binding`); + + renderer.component.add_var({ + name, + internal: true, + referenced: true + }); + + const { handler, object } = this_binding; + + const args = []; + for (const arg of handler.contextual_dependencies) { + args.push(arg); + block.add_variable(arg, `ctx.${arg}`); } + + renderer.component.partly_hoisted.push(deindent` + function ${name}(${['$$node', 'check'].concat(args).join(', ')}) { + ${handler.snippet ? `if ($$node || (!$$node && ${handler.snippet} === check)) ` : ''}${handler.mutation} + ${renderer.component.invalidate(object)}; + } + `); + + block.builders.mount.add_line(`@add_binding_callback(() => ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}));`); + block.builders.destroy.add_line(`ctx.${name}(${['null', this.var].concat(args).join(', ')});`); + block.builders.update.add_line(deindent` + if (changed.items) { + ctx.${name}(${['null', this.var].concat(args).join(', ')}); + ${args.map(a => `${a} = ctx.${a}`).join(', ')}; + ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}); + }` + ); } add_attributes(block: Block) { diff --git a/test/js/samples/media-bindings/expected.js b/test/js/samples/media-bindings/expected.js index 8a193f698b29..94444b8947da 100644 --- a/test/js/samples/media-bindings/expected.js +++ b/test/js/samples/media-bindings/expected.js @@ -26,18 +26,18 @@ function create_fragment(ctx) { return { c() { audio = element("audio"); - if (ctx.played === void 0 || ctx.currentTime === void 0) add_render_callback(audio_timeupdate_handler); - if (ctx.duration === void 0) add_render_callback(() => ctx.audio_durationchange_handler.call(audio)); if (ctx.buffered === void 0) add_render_callback(() => ctx.audio_progress_handler.call(audio)); if (ctx.buffered === void 0 || ctx.seekable === void 0) add_render_callback(() => ctx.audio_loadedmetadata_handler.call(audio)); + if (ctx.played === void 0 || ctx.currentTime === void 0) add_render_callback(audio_timeupdate_handler); + if (ctx.duration === void 0) add_render_callback(() => ctx.audio_durationchange_handler.call(audio)); dispose = [ + listen(audio, "progress", ctx.audio_progress_handler), + listen(audio, "loadedmetadata", ctx.audio_loadedmetadata_handler), listen(audio, "timeupdate", audio_timeupdate_handler), listen(audio, "durationchange", ctx.audio_durationchange_handler), listen(audio, "play", ctx.audio_play_pause_handler), listen(audio, "pause", ctx.audio_play_pause_handler), - listen(audio, "progress", ctx.audio_progress_handler), - listen(audio, "loadedmetadata", ctx.audio_loadedmetadata_handler), listen(audio, "volumechange", ctx.audio_volumechange_handler), listen(audio, "ratechange", ctx.audio_ratechange_handler) ]; @@ -53,10 +53,10 @@ function create_fragment(ctx) { p(changed, ctx) { if (!audio_updating && changed.currentTime && !isNaN(ctx.currentTime)) audio.currentTime = ctx.currentTime; + audio_updating = false; if (changed.paused && audio_is_paused !== (audio_is_paused = ctx.paused)) audio[audio_is_paused ? "pause" : "play"](); if (changed.volume && !isNaN(ctx.volume)) audio.volume = ctx.volume; if (changed.playbackRate && !isNaN(ctx.playbackRate)) audio.playbackRate = ctx.playbackRate; - audio_updating = false; }, i: noop, @@ -75,6 +75,18 @@ function create_fragment(ctx) { function instance($$self, $$props, $$invalidate) { let { buffered, seekable, played, currentTime, duration, paused, volume, playbackRate } = $$props; + function audio_progress_handler() { + buffered = time_ranges_to_array(this.buffered); + $$invalidate('buffered', buffered); + } + + function audio_loadedmetadata_handler() { + buffered = time_ranges_to_array(this.buffered); + seekable = time_ranges_to_array(this.seekable); + $$invalidate('buffered', buffered); + $$invalidate('seekable', seekable); + } + function audio_timeupdate_handler() { played = time_ranges_to_array(this.played); currentTime = this.currentTime; @@ -92,18 +104,6 @@ function instance($$self, $$props, $$invalidate) { $$invalidate('paused', paused); } - function audio_progress_handler() { - buffered = time_ranges_to_array(this.buffered); - $$invalidate('buffered', buffered); - } - - function audio_loadedmetadata_handler() { - buffered = time_ranges_to_array(this.buffered); - seekable = time_ranges_to_array(this.seekable); - $$invalidate('buffered', buffered); - $$invalidate('seekable', seekable); - } - function audio_volumechange_handler() { volume = this.volume; $$invalidate('volume', volume); @@ -134,11 +134,11 @@ function instance($$self, $$props, $$invalidate) { paused, volume, playbackRate, + audio_progress_handler, + audio_loadedmetadata_handler, audio_timeupdate_handler, audio_durationchange_handler, audio_play_pause_handler, - audio_progress_handler, - audio_loadedmetadata_handler, audio_volumechange_handler, audio_ratechange_handler }; @@ -151,4 +151,4 @@ class Component extends SvelteComponent { } } -export default Component; \ No newline at end of file +export default Component; diff --git a/test/runtime/samples/apply-directives-in-order/_config.js b/test/runtime/samples/apply-directives-in-order/_config.js new file mode 100644 index 000000000000..e5e8980ed115 --- /dev/null +++ b/test/runtime/samples/apply-directives-in-order/_config.js @@ -0,0 +1,37 @@ +export default { + props: { + value: '' + }, + + html: ` + +

+ `, + + ssrHtml: ` + +

+ `, + + async test({ assert, component, target, window }) { + const input = target.querySelector('input'); + + const event = new window.Event('input'); + input.value = 'h'; + await input.dispatchEvent(event); + + assert.equal(input.value, 'H'); + assert.htmlEqual(target.innerHTML, ` + +

H

+ `); + + input.value = 'he'; + await input.dispatchEvent(event); + assert.equal(input.value, 'HE'); + assert.htmlEqual(target.innerHTML, ` + +

HE

+ `); + }, +}; diff --git a/test/runtime/samples/apply-directives-in-order/main.svelte b/test/runtime/samples/apply-directives-in-order/main.svelte new file mode 100644 index 000000000000..be652c7b7956 --- /dev/null +++ b/test/runtime/samples/apply-directives-in-order/main.svelte @@ -0,0 +1,10 @@ + + + +

{value}