Skip to content

harden: idempotent destroy(), sanitize wrapper classNames, partial-init safety #479

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.

Sign up for GitHub

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

Merged
gnbm merged 1 commit into from
Jun 9, 2026
Merged

harden: idempotent destroy(), sanitize wrapper classNames, partial-init safety #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 48 additions & 0 deletions cypress/e2e/observer-listener-lifecycle.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,52 @@ describe('Lifecycle: ref-counted observer/listeners + throttle cancel', () => {
});
});
});

it('destroy() is idempotent - calling it twice is a safe no-op', () => {
cy.visit('get-started');
cy.window().then((win) => {
const VS = (win as unknown as { VirtualSelect: any }).VirtualSelect;

destroyAll(win);
const $ele = mount(win);
const instance = $ele.virtualSelect;

// First destroy tears the instance down; the second must not throw and must not
// corrupt the global teardown state (e.g. re-detach listeners or go negative).
expect(() => {
instance.destroy();
instance.destroy();
}, 'second destroy() does not throw').to.not.throw();

expect(instance.isDestroyed, 'instance flagged destroyed').to.eq(true);
expect(VS.activeInstances.size, 'no instances remain after double-destroy').to.eq(0);
expect(VS.hasGlobalListeners, 'global-listeners flag cleared').to.eq(false);
expect(VS.domObserver, 'shared observer disconnected and nulled').to.eq(null);
});
});

it('shared observer auto-destroys an instance whose host is removed without destroy()', () => {
cy.visit('get-started');
cy.window().then((win) => {
const VS = (win as unknown as { VirtualSelect: any }).VirtualSelect;

// Reach a single-instance state so removing this host empties activeInstances and
// the observer/listeners must tear down on their own (no explicit destroy() call).
destroyAll(win);
const $ele = mount(win);
expect($ele.virtualSelect, 'instance attached to host').to.not.eq(undefined);
expect(VS.activeInstances.size, 'exactly one live instance').to.eq(1);

// Detach the host from the DOM directly - this is the observer's primary purpose.
$ele.remove();

// MutationObserver callbacks are delivered asynchronously; use retried assertions.
cy.wrap(null).should(() => {
expect($ele.virtualSelect, 'instance auto-destroyed (back-reference cleared)').to.eq(undefined);
expect(VS.activeInstances.size, 'instance untracked after host removal').to.eq(0);
expect(VS.hasGlobalListeners, 'page listeners torn down with the last instance').to.eq(false);
expect(VS.domObserver, 'shared observer disconnected once no instance remains').to.eq(null);
});
});
});
});
2 changes: 1 addition & 1 deletion dist-archive/virtual-select-1.2.0.min.js
View file
Open in desktop

Large diffs are not rendered by default.

59 changes: 38 additions & 21 deletions dist/virtual-select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,7 @@ class VirtualSelect {
* @param {virtualSelectOptions} options
*/
constructor(options) {
this.isDestroyed = false;
try {
this.createSecureTextElements();
this.setProps(options);
Expand Down Expand Up @@ -708,10 +709,10 @@ class VirtualSelect {
const ariaLabelClearBtnTxt = this.ariaLabelClearButtonText ? `aria-label="${this.ariaLabelClearButtonText}"` : '';
let isExpanded = false;
if (this.additionalClasses) {
wrapperClasses += ` ${this.additionalClasses}`;
wrapperClasses += ` ${Utils.sanitizeClassNames(this.additionalClasses)}`;
}
if (this.additionalToggleButtonClasses) {
toggleButtonClasses += ` ${this.additionalToggleButtonClasses}`;
toggleButtonClasses += ` ${Utils.sanitizeClassNames(this.additionalToggleButtonClasses)}`;
}
if (this.multiple) {
wrapperClasses += ' multiple';
Expand Down Expand Up @@ -797,11 +798,11 @@ class VirtualSelect {
const $wrapper = this.dropboxWrapper !== 'self' ? document.querySelector(this.dropboxWrapper) : null;
let dropboxClasses = 'vscomp-dropbox';
if (this.additionalDropboxClasses) {
dropboxClasses += ` ${this.additionalDropboxClasses}`;
dropboxClasses += ` ${Utils.sanitizeClassNames(this.additionalDropboxClasses)}`;
}
let dropboxContainerClasses = 'vscomp-dropbox-container';
if (this.additionalDropboxContainerClasses) {
dropboxContainerClasses += ` ${this.additionalDropboxContainerClasses}`;
dropboxContainerClasses += ` ${Utils.sanitizeClassNames(this.additionalDropboxContainerClasses)}`;
}

// eslint-disable-next-line no-trailing-spaces
Expand Down Expand Up @@ -1423,23 +1424,35 @@ class VirtualSelect {
this.setVisibleOptionsCount();
this.setOptionsContainerHeight();
this.addEvents();
this.setEleProps();
if (!this.keepAlwaysOpen && !this.showAsPopup) {
this.initDropboxPopover();
}
if (this.initialSelectedValue) {
this.setValueMethod(this.initialSelectedValue, this.silentInitialValueSet);
} else if (this.autoSelectFirstOption && this.visibleOptions.length) {
this.setValueMethod(this.visibleOptions[0].value, this.silentInitialValueSet);
}
if (this.showOptionsOnlyOnSearch) {
this.setSearchValue('', false, true);
}
if (this.initialDisabled) {
this.disable();
}
if (this.autofocus) {
this.focus();

/**
* addEvents() registers this instance (installing the shared observer and the page-level
* listeners). If any of the steps below throw, the instance is registered but the caller
* has no handle to destroy() it, leaking the global listeners/observer. Self-destroy on
* failure and rethrow so the constructor's existing handling still reports the error.
*/
try {
this.setEleProps();
if (!this.keepAlwaysOpen && !this.showAsPopup) {
this.initDropboxPopover();
}
if (this.initialSelectedValue) {
this.setValueMethod(this.initialSelectedValue, this.silentInitialValueSet);
} else if (this.autoSelectFirstOption && this.visibleOptions.length) {
this.setValueMethod(this.visibleOptions[0].value, this.silentInitialValueSet);
}
if (this.showOptionsOnlyOnSearch) {
this.setSearchValue('', false, true);
}
if (this.initialDisabled) {
this.disable();
}
if (this.autofocus) {
this.focus();
}
} catch (e) {
this.destroy();
throw e;
}
}
afterRenderOptions() {
Expand Down Expand Up @@ -3673,6 +3686,10 @@ class VirtualSelect {
}
}
destroy() {
if (this.isDestroyed) {
return;
}
this.isDestroyed = true;
const {
$ele
} = this;
Expand Down
2 changes: 1 addition & 1 deletion dist/virtual-select.min.js
View file
Open in desktop

Large diffs are not rendered by default.

59 changes: 38 additions & 21 deletions docs/assets/virtual-select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,7 @@ class VirtualSelect {
* @param {virtualSelectOptions} options
*/
constructor(options) {
this.isDestroyed = false;
try {
Comment thread
gnbm marked this conversation as resolved.
this.createSecureTextElements();
this.setProps(options);
Expand Down Expand Up @@ -708,10 +709,10 @@ class VirtualSelect {
const ariaLabelClearBtnTxt = this.ariaLabelClearButtonText ? `aria-label="${this.ariaLabelClearButtonText}"` : '';
let isExpanded = false;
if (this.additionalClasses) {
wrapperClasses += ` ${this.additionalClasses}`;
wrapperClasses += ` ${Utils.sanitizeClassNames(this.additionalClasses)}`;
}
if (this.additionalToggleButtonClasses) {
toggleButtonClasses += ` ${this.additionalToggleButtonClasses}`;
toggleButtonClasses += ` ${Utils.sanitizeClassNames(this.additionalToggleButtonClasses)}`;
}
if (this.multiple) {
wrapperClasses += ' multiple';
Expand Down Expand Up @@ -797,11 +798,11 @@ class VirtualSelect {
const $wrapper = this.dropboxWrapper !== 'self' ? document.querySelector(this.dropboxWrapper) : null;
let dropboxClasses = 'vscomp-dropbox';
if (this.additionalDropboxClasses) {
dropboxClasses += ` ${this.additionalDropboxClasses}`;
dropboxClasses += ` ${Utils.sanitizeClassNames(this.additionalDropboxClasses)}`;
}
let dropboxContainerClasses = 'vscomp-dropbox-container';
if (this.additionalDropboxContainerClasses) {
dropboxContainerClasses += ` ${this.additionalDropboxContainerClasses}`;
dropboxContainerClasses += ` ${Utils.sanitizeClassNames(this.additionalDropboxContainerClasses)}`;
}

// eslint-disable-next-line no-trailing-spaces
Expand Down Expand Up @@ -1423,23 +1424,35 @@ class VirtualSelect {
this.setVisibleOptionsCount();
this.setOptionsContainerHeight();
this.addEvents();
this.setEleProps();
if (!this.keepAlwaysOpen && !this.showAsPopup) {
this.initDropboxPopover();
}
if (this.initialSelectedValue) {
this.setValueMethod(this.initialSelectedValue, this.silentInitialValueSet);
} else if (this.autoSelectFirstOption && this.visibleOptions.length) {
this.setValueMethod(this.visibleOptions[0].value, this.silentInitialValueSet);
}
if (this.showOptionsOnlyOnSearch) {
this.setSearchValue('', false, true);
}
if (this.initialDisabled) {
this.disable();
}
if (this.autofocus) {
this.focus();

/**
* addEvents() registers this instance (installing the shared observer and the page-level
* listeners). If any of the steps below throw, the instance is registered but the caller
* has no handle to destroy() it, leaking the global listeners/observer. Self-destroy on
* failure and rethrow so the constructor's existing handling still reports the error.
*/
try {
this.setEleProps();
if (!this.keepAlwaysOpen && !this.showAsPopup) {
this.initDropboxPopover();
}
if (this.initialSelectedValue) {
this.setValueMethod(this.initialSelectedValue, this.silentInitialValueSet);
} else if (this.autoSelectFirstOption && this.visibleOptions.length) {
this.setValueMethod(this.visibleOptions[0].value, this.silentInitialValueSet);
}
if (this.showOptionsOnlyOnSearch) {
this.setSearchValue('', false, true);
}
if (this.initialDisabled) {
this.disable();
}
if (this.autofocus) {
this.focus();
}
} catch (e) {
this.destroy();
throw e;
}
}
afterRenderOptions() {
Expand Down Expand Up @@ -3673,6 +3686,10 @@ class VirtualSelect {
}
}
destroy() {
if (this.isDestroyed) {
return;
}
this.isDestroyed = true;
const {
$ele
} = this;
Expand Down
2 changes: 1 addition & 1 deletion docs/assets/virtual-select.min.js
View file
Open in desktop

Large diffs are not rendered by default.

61 changes: 40 additions & 21 deletions src/virtual-select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ export class VirtualSelect {
* @param {virtualSelectOptions} options
*/
constructor(options) {
this.isDestroyed = false;

try {
this.createSecureTextElements();
this.setProps(options);
Expand Down Expand Up @@ -127,11 +129,11 @@ export class VirtualSelect {
let isExpanded = false;

if (this.additionalClasses) {
wrapperClasses += ` ${this.additionalClasses}`;
wrapperClasses += ` ${Utils.sanitizeClassNames(this.additionalClasses)}`;
}

if (this.additionalToggleButtonClasses) {
toggleButtonClasses += ` ${this.additionalToggleButtonClasses}`;
toggleButtonClasses += ` ${Utils.sanitizeClassNames(this.additionalToggleButtonClasses)}`;
}

if (this.multiple) {
Expand Down Expand Up @@ -230,13 +232,13 @@ export class VirtualSelect {
let dropboxClasses = 'vscomp-dropbox';

if (this.additionalDropboxClasses) {
dropboxClasses += ` ${this.additionalDropboxClasses}`;
dropboxClasses += ` ${Utils.sanitizeClassNames(this.additionalDropboxClasses)}`;
}

let dropboxContainerClasses = 'vscomp-dropbox-container';

if (this.additionalDropboxContainerClasses) {
dropboxContainerClasses += ` ${this.additionalDropboxContainerClasses}`;
dropboxContainerClasses += ` ${Utils.sanitizeClassNames(this.additionalDropboxContainerClasses)}`;
}

// eslint-disable-next-line no-trailing-spaces
Expand Down Expand Up @@ -940,28 +942,40 @@ export class VirtualSelect {
this.setVisibleOptionsCount();
this.setOptionsContainerHeight();
this.addEvents();
this.setEleProps();

if (!this.keepAlwaysOpen && !this.showAsPopup) {
this.initDropboxPopover();
}
/**
* addEvents() registers this instance (installing the shared observer and the page-level
* listeners). If any of the steps below throw, the instance is registered but the caller
* has no handle to destroy() it, leaking the global listeners/observer. Self-destroy on
* failure and rethrow so the constructor's existing handling still reports the error.
*/
try {
this.setEleProps();

if (this.initialSelectedValue) {
this.setValueMethod(this.initialSelectedValue, this.silentInitialValueSet);
} else if (this.autoSelectFirstOption && this.visibleOptions.length) {
this.setValueMethod(this.visibleOptions[0].value, this.silentInitialValueSet);
}
if (!this.keepAlwaysOpen && !this.showAsPopup) {
this.initDropboxPopover();
}

if (this.showOptionsOnlyOnSearch) {
this.setSearchValue('', false, true);
}
if (this.initialSelectedValue) {
this.setValueMethod(this.initialSelectedValue, this.silentInitialValueSet);
} else if (this.autoSelectFirstOption && this.visibleOptions.length) {
this.setValueMethod(this.visibleOptions[0].value, this.silentInitialValueSet);
}

if (this.initialDisabled) {
this.disable();
}
if (this.showOptionsOnlyOnSearch) {
this.setSearchValue('', false, true);
}

if (this.initialDisabled) {
this.disable();
}

if (this.autofocus) {
this.focus();
if (this.autofocus) {
this.focus();
}
} catch (e) {
this.destroy();
throw e;
}
}

Expand Down Expand Up @@ -3548,6 +3562,11 @@ export class VirtualSelect {
}

destroy() {
if (this.isDestroyed) {
return;
}
this.isDestroyed = true;

const { $ele } = this;
$ele.virtualSelect = undefined;
$ele.value = undefined;
Expand Down