From 6b02cf5d46e0fabd9ee1a2b2373382b9455ff1af Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Thu, 6 Aug 2020 18:31:26 +0100 Subject: [PATCH 1/4] feat(GlFormInput): Add debounce/lazy support The underlying `BFormInput` supports a `debounce` prop and a `lazy` prop, allowing for debounced and lazy model updates respectively. Because Vue's default model event is `input`, and `BFormInput`'s model event is `update`, we weren't able to make use of this before, since the debounced/lazy effects of `BFormInput` were implemented via its `update` event, _not_ its `input` event. Model binding on `GlFormInput` _happened_ to work prior to this because `BFormInput` also always synchronously emits `input` events for _all_ native `input` DOM events. One way to fix this would be to make `GlFormInput`'s `model` match the underlying `BFormInput`'s `model`, i.e., change `model.event` to `update`, rather than the implicit default `input`. The problem with this is that this is effectively a breaking change in our API. Instead, we make `GlFormInput`'s `model.event` _explicitly_ `input`, and map `update` events from the underlying `BFormInput` to `input` events emitted by `GlFormInput`, and vice versa. This is not a breaking change. Addresses https://gitlab.com/gitlab-org/gitlab-ui/-/issues/631. --- .../base/form/form_input/form_input.spec.js | 118 ++++++++++++++++-- .../base/form/form_input/form_input.vue | 21 +++- .../search_box_by_type/search_box_by_type.vue | 6 + 3 files changed, 137 insertions(+), 8 deletions(-) diff --git a/src/components/base/form/form_input/form_input.spec.js b/src/components/base/form/form_input/form_input.spec.js index f0091e6098..471e33397a 100644 --- a/src/components/base/form/form_input/form_input.spec.js +++ b/src/components/base/form/form_input/form_input.spec.js @@ -1,14 +1,15 @@ -import { shallowMount } from '@vue/test-utils'; +import { mount, shallowMount } from '@vue/test-utils'; import GlFormInput from './form_input.vue'; import { formInputSizes } from '../../../../utils/constants'; +const modelEvent = GlFormInput.model.event; +const newValue = 'foo'; + describe('GlFormInput', () => { let wrapper; - const createComponent = propsData => { - wrapper = shallowMount(GlFormInput, { - propsData, - }); + const createComponent = (options, mountFn = shallowMount) => { + wrapper = mountFn(GlFormInput, options); }; afterEach(() => { @@ -21,7 +22,7 @@ describe('GlFormInput', () => { const sizes = Object.values(formInputSizes).filter(Boolean); it.each(sizes)('adds correct class for size %s', size => { - createComponent({ size }); + createComponent({ propsData: { size } }); expect(wrapper.classes()).toEqual(['gl-form-input', `gl-form-input-${size}`]); }); @@ -33,9 +34,112 @@ describe('GlFormInput', () => { }); it('does not add a size class if passed null', () => { - createComponent({ size: null }); + createComponent({ propsData: { size: null } }); expect(wrapper.classes()).toEqual(['gl-form-input']); }); }); + + describe('debounce', () => { + let modelEventSpy; + let updateEventSpy; + let listeners; + + beforeEach(() => { + jest.useFakeTimers(); + modelEventSpy = jest.fn(); + updateEventSpy = jest.fn(); + listeners = { + [modelEvent]: modelEventSpy, + update: updateEventSpy, + }; + }); + + describe.each([10, 100, 1000])('given a debounce of %dms', debounce => { + beforeEach(() => { + createComponent( + { + propsData: { debounce }, + listeners, + }, + mount + ); + + wrapper.setValue(newValue); + }); + + it('synchronously emits an update event', () => { + expect(updateEventSpy.mock.calls).toEqual([[newValue]]); + }); + + it('emits a model event after the debounce delay', () => { + // Just before debounce completes + jest.advanceTimersByTime(debounce - 1); + expect(modelEventSpy).not.toHaveBeenCalled(); + + // Exactly when debounce completes + jest.advanceTimersByTime(1); + expect(modelEventSpy.mock.calls).toEqual([[newValue]]); + }); + }); + + describe('given a debounce of 0', () => { + beforeEach(() => { + createComponent( + { + propsData: { debounce: 0 }, + listeners, + }, + mount + ); + + wrapper.setValue(newValue); + }); + + it('synchronously emits an update event', () => { + expect(updateEventSpy.mock.calls).toEqual([[newValue]]); + }); + + it('synchronously updates model if debounce is 0', () => { + expect(modelEventSpy.mock.calls).toEqual([[newValue]]); + }); + }); + }); + + describe('lazy', () => { + let modelEventSpy; + let updateEventSpy; + let listeners; + + beforeEach(() => { + jest.useFakeTimers(); + modelEventSpy = jest.fn(); + updateEventSpy = jest.fn(); + listeners = { + [modelEvent]: modelEventSpy, + update: updateEventSpy, + }; + + createComponent( + { + propsData: { lazy: true }, + listeners, + }, + mount + ); + + wrapper.setValue(newValue); + }); + + it('synchronously emits an update event', () => { + expect(updateEventSpy.mock.calls).toEqual([[newValue]]); + }); + + it.each(['change', 'blur'])('updates model after %s event', event => { + expect(modelEventSpy).not.toHaveBeenCalled(); + + wrapper.trigger(event); + expect(modelEventSpy.mock.calls).toEqual([[newValue]]); + }); + }); }); diff --git a/src/components/base/form/form_input/form_input.vue b/src/components/base/form/form_input/form_input.vue index 0fa3b099ad..88cde3d172 100644 --- a/src/components/base/form/form_input/form_input.vue +++ b/src/components/base/form/form_input/form_input.vue @@ -2,11 +2,17 @@ import { BFormInput } from 'bootstrap-vue'; import { formInputSizes } from '../../../../utils/constants'; +const model = { + prop: 'value', + event: 'input', +}; + export default { components: { BFormInput, }, inheritAttrs: false, + model, props: { size: { type: String, @@ -21,10 +27,23 @@ export default { [`gl-form-input-${this.size}`]: this.size !== null, }; }, + listeners() { + return { + ...this.$listeners, + // Swap purpose of input and update events from underlying BFormInput. + // See https://gitlab.com/gitlab-org/gitlab-ui/-/issues/631. + input: (...args) => { + this.$emit('update', ...args); + }, + update: (...args) => { + this.$emit(model.event, ...args); + }, + }; + }, }, }; diff --git a/src/components/base/search_box_by_type/search_box_by_type.vue b/src/components/base/search_box_by_type/search_box_by_type.vue index 8bd37a18dc..c26df9be43 100644 --- a/src/components/base/search_box_by_type/search_box_by_type.vue +++ b/src/components/base/search_box_by_type/search_box_by_type.vue @@ -4,6 +4,11 @@ import GlIcon from '../icon/icon.vue'; import GlFormInput from '../form/form_input/form_input.vue'; import GlLoadingIcon from '../loading_icon/loading_icon.vue'; +const model = { + prop: 'value', + event: 'input', +}; + export default { components: { GlClearIconButton, @@ -12,6 +17,7 @@ export default { GlLoadingIcon, }, inheritAttrs: false, + model, props: { value: { type: String, -- GitLab From d09eb0150cd8959409dd9e0249025aa982f080d7 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Tue, 11 Aug 2020 17:34:40 +0100 Subject: [PATCH 2/4] Use wrapper.emitted rather than spies --- .../base/form/form_input/form_input.spec.js | 75 +++++-------------- 1 file changed, 20 insertions(+), 55 deletions(-) diff --git a/src/components/base/form/form_input/form_input.spec.js b/src/components/base/form/form_input/form_input.spec.js index 471e33397a..06b0b7bb16 100644 --- a/src/components/base/form/form_input/form_input.spec.js +++ b/src/components/base/form/form_input/form_input.spec.js @@ -8,8 +8,10 @@ const newValue = 'foo'; describe('GlFormInput', () => { let wrapper; - const createComponent = (options, mountFn = shallowMount) => { - wrapper = mountFn(GlFormInput, options); + const createComponent = (propsData = {}, mountFn = shallowMount) => { + wrapper = mountFn(GlFormInput, { + propsData, + }); }; afterEach(() => { @@ -22,7 +24,7 @@ describe('GlFormInput', () => { const sizes = Object.values(formInputSizes).filter(Boolean); it.each(sizes)('adds correct class for size %s', size => { - createComponent({ propsData: { size } }); + createComponent({ size }); expect(wrapper.classes()).toEqual(['gl-form-input', `gl-form-input-${size}`]); }); @@ -34,112 +36,75 @@ describe('GlFormInput', () => { }); it('does not add a size class if passed null', () => { - createComponent({ propsData: { size: null } }); + createComponent({ size: null }); expect(wrapper.classes()).toEqual(['gl-form-input']); }); }); describe('debounce', () => { - let modelEventSpy; - let updateEventSpy; - let listeners; - beforeEach(() => { jest.useFakeTimers(); - modelEventSpy = jest.fn(); - updateEventSpy = jest.fn(); - listeners = { - [modelEvent]: modelEventSpy, - update: updateEventSpy, - }; }); describe.each([10, 100, 1000])('given a debounce of %dms', debounce => { beforeEach(() => { - createComponent( - { - propsData: { debounce }, - listeners, - }, - mount - ); + createComponent({ debounce }, mount); wrapper.setValue(newValue); }); it('synchronously emits an update event', () => { - expect(updateEventSpy.mock.calls).toEqual([[newValue]]); + expect(wrapper.emitted('update')).toEqual([[newValue]]); }); it('emits a model event after the debounce delay', () => { // Just before debounce completes jest.advanceTimersByTime(debounce - 1); - expect(modelEventSpy).not.toHaveBeenCalled(); + expect(wrapper.emitted(modelEvent)).toBe(undefined); // Exactly when debounce completes jest.advanceTimersByTime(1); - expect(modelEventSpy.mock.calls).toEqual([[newValue]]); + expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); }); }); describe('given a debounce of 0', () => { beforeEach(() => { - createComponent( - { - propsData: { debounce: 0 }, - listeners, - }, - mount - ); + createComponent({ debounce: 0 }, mount); wrapper.setValue(newValue); }); it('synchronously emits an update event', () => { - expect(updateEventSpy.mock.calls).toEqual([[newValue]]); + expect(wrapper.emitted('update')).toEqual([[newValue]]); }); - it('synchronously updates model if debounce is 0', () => { - expect(modelEventSpy.mock.calls).toEqual([[newValue]]); + it('synchronously updates model', () => { + expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); }); }); }); describe('lazy', () => { - let modelEventSpy; - let updateEventSpy; - let listeners; - beforeEach(() => { jest.useFakeTimers(); - modelEventSpy = jest.fn(); - updateEventSpy = jest.fn(); - listeners = { - [modelEvent]: modelEventSpy, - update: updateEventSpy, - }; - - createComponent( - { - propsData: { lazy: true }, - listeners, - }, - mount - ); + + createComponent({ lazy: true }, mount); wrapper.setValue(newValue); }); it('synchronously emits an update event', () => { - expect(updateEventSpy.mock.calls).toEqual([[newValue]]); + expect(wrapper.emitted('update')).toEqual([[newValue]]); }); it.each(['change', 'blur'])('updates model after %s event', event => { - expect(modelEventSpy).not.toHaveBeenCalled(); + expect(wrapper.emitted(modelEvent)).toBe(undefined); wrapper.trigger(event); - expect(modelEventSpy.mock.calls).toEqual([[newValue]]); + + expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); }); }); }); -- GitLab From b5a10d0b9dbb0ec49d0dd0fa9d3ee103ae031ae7 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Wed, 12 Aug 2020 10:00:52 +0100 Subject: [PATCH 3/4] Add debounce/lazy specs for SearchByType --- .../base/form/form_input/form_input.spec.js | 8 +- .../search_box_by_type.spec.js | 86 ++++++++++++++++--- 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/src/components/base/form/form_input/form_input.spec.js b/src/components/base/form/form_input/form_input.spec.js index 06b0b7bb16..6b96b8a4d8 100644 --- a/src/components/base/form/form_input/form_input.spec.js +++ b/src/components/base/form/form_input/form_input.spec.js @@ -43,12 +43,10 @@ describe('GlFormInput', () => { }); describe('debounce', () => { - beforeEach(() => { - jest.useFakeTimers(); - }); - describe.each([10, 100, 1000])('given a debounce of %dms', debounce => { beforeEach(() => { + jest.useFakeTimers(); + createComponent({ debounce }, mount); wrapper.setValue(newValue); @@ -88,8 +86,6 @@ describe('GlFormInput', () => { describe('lazy', () => { beforeEach(() => { - jest.useFakeTimers(); - createComponent({ lazy: true }, mount); wrapper.setValue(newValue); diff --git a/src/components/base/search_box_by_type/search_box_by_type.spec.js b/src/components/base/search_box_by_type/search_box_by_type.spec.js index b78396c048..6c065d26a1 100644 --- a/src/components/base/search_box_by_type/search_box_by_type.spec.js +++ b/src/components/base/search_box_by_type/search_box_by_type.spec.js @@ -1,26 +1,31 @@ -import { shallowMount } from '@vue/test-utils'; +import { mount, shallowMount } from '@vue/test-utils'; import SearchBoxByType from './search_box_by_type.vue'; import LoadingIcon from '../loading_icon/loading_icon.vue'; import ClearIcon from '~/components/shared_components/clear_icon_button/clear_icon_button.vue'; +const modelEvent = SearchBoxByType.model.event; +const newValue = 'new value'; + describe('search box by type component', () => { let wrapper; - const createComponent = propsData => { - wrapper = shallowMount(SearchBoxByType, { propsData }); + const createComponent = (propsData, mountFn = shallowMount) => { + wrapper = mountFn(SearchBoxByType, { propsData }); }; const findClearIcon = () => wrapper.find(ClearIcon); - - beforeEach(() => { - createComponent({ value: 'somevalue' }); - }); + const findInput = () => wrapper.find({ ref: 'input' }); afterEach(() => { wrapper.destroy(); + wrapper = null; }); describe('clear icon component', () => { + beforeEach(() => { + createComponent({ value: 'somevalue' }); + }); + it('is not rendered when value is empty', () => { createComponent({ value: '' }); expect(findClearIcon().exists()).toBe(false); @@ -38,16 +43,71 @@ describe('search box by type component', () => { }); describe('v-model', () => { - it('syncs localValue to value prop', () => { - wrapper.setProps({ value: 'new value' }); + beforeEach(() => { + createComponent({ value: 'somevalue' }, mount); + }); + + it('syncs value prop to input value', async () => { + wrapper.setProps({ value: newValue }); + await wrapper.vm.$nextTick(); + + expect(findInput().element.value).toEqual(newValue); + }); + + it(`emits ${modelEvent} event when input value changes`, () => { + findInput().setValue(newValue); + + expect(wrapper.emitted().input).toEqual([[newValue]]); + }); + }); - expect(wrapper.vm.localValue).toEqual('new value'); + describe('debounce', () => { + describe.each([10, 100, 1000])('given a debounce of %dms', debounce => { + beforeEach(() => { + jest.useFakeTimers(); + + createComponent({ debounce }, mount); + + findInput().setValue(newValue); + }); + + it(`emits a ${modelEvent} after the debounce delay`, () => { + // Just before debounce completes + jest.advanceTimersByTime(debounce - 1); + expect(wrapper.emitted(modelEvent)).toBe(undefined); + + // Exactly when debounce completes + jest.advanceTimersByTime(1); + expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); + }); }); - it('emits input event when localValue changes', () => { - wrapper.vm.localValue = 'new value'; + describe('given a debounce of 0', () => { + beforeEach(() => { + createComponent({ debounce: 0 }, mount); + + findInput().setValue(newValue); + }); + + it(`synchronously emits a ${modelEvent} event`, () => { + expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); + }); + }); + }); + + describe('lazy', () => { + beforeEach(() => { + createComponent({ lazy: true }, mount); + + findInput().setValue(newValue); + }); + + it.each(['change', 'blur'])(`emits ${modelEvent} event after input's %s event`, event => { + expect(wrapper.emitted(modelEvent)).toBe(undefined); + + findInput().trigger(event); - expect(wrapper.emitted().input).toEqual([['new value']]); + expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); }); }); -- GitLab From 41f95eac50a8ae3f824d57e3af213a9418f286e9 Mon Sep 17 00:00:00 2001 From: Mark Florian Date: Wed, 12 Aug 2020 17:32:59 +0100 Subject: [PATCH 4/4] Extract default model event behaviour tests The `debounce: 0` cases are now tested in the `v-model` describe blocks, where no debounce prop is given, so it takes on its default of 0. The `v-model` blocks also implicitly test the `lazy: false` case, since that's its default value as well. --- .../base/form/form_input/form_input.spec.js | 32 +++++++++---------- .../search_box_by_type.spec.js | 12 ------- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/src/components/base/form/form_input/form_input.spec.js b/src/components/base/form/form_input/form_input.spec.js index 6b96b8a4d8..f318157dca 100644 --- a/src/components/base/form/form_input/form_input.spec.js +++ b/src/components/base/form/form_input/form_input.spec.js @@ -42,6 +42,22 @@ describe('GlFormInput', () => { }); }); + describe('v-model', () => { + beforeEach(() => { + createComponent({}, mount); + + wrapper.setValue(newValue); + }); + + it('synchronously emits an update event', () => { + expect(wrapper.emitted('update')).toEqual([[newValue]]); + }); + + it('synchronously updates model', () => { + expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); + }); + }); + describe('debounce', () => { describe.each([10, 100, 1000])('given a debounce of %dms', debounce => { beforeEach(() => { @@ -66,22 +82,6 @@ describe('GlFormInput', () => { expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); }); }); - - describe('given a debounce of 0', () => { - beforeEach(() => { - createComponent({ debounce: 0 }, mount); - - wrapper.setValue(newValue); - }); - - it('synchronously emits an update event', () => { - expect(wrapper.emitted('update')).toEqual([[newValue]]); - }); - - it('synchronously updates model', () => { - expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); - }); - }); }); describe('lazy', () => { diff --git a/src/components/base/search_box_by_type/search_box_by_type.spec.js b/src/components/base/search_box_by_type/search_box_by_type.spec.js index 6c065d26a1..63a887bc6c 100644 --- a/src/components/base/search_box_by_type/search_box_by_type.spec.js +++ b/src/components/base/search_box_by_type/search_box_by_type.spec.js @@ -81,18 +81,6 @@ describe('search box by type component', () => { expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); }); }); - - describe('given a debounce of 0', () => { - beforeEach(() => { - createComponent({ debounce: 0 }, mount); - - findInput().setValue(newValue); - }); - - it(`synchronously emits a ${modelEvent} event`, () => { - expect(wrapper.emitted(modelEvent)).toEqual([[newValue]]); - }); - }); }); describe('lazy', () => { -- GitLab