Skip to content

Commit 0f46817

Browse files
committed
Lock the type surface with access modifiers and honest nullability
The migration exposed every component internal as public. Consumers could reach `_config`, `_element`, and helper methods, which would make them breaking changes once shipped. Restrict the surface and stop a few types from lying about nullability. - Mark every `_`-prefixed instance field and method `protected` across all components (409 members). `protected`, not `private`, so subclasses keep access. Public API methods and statics stay public. Two carousel data-api hooks stay public because the module-level handlers call them. - Retype `getOrCreateInstance`'s config to `NonNullable<ConstructorParameters<T>[1]>` so it still enforces each component's config now that `_config` is protected (indexing a protected member on a type parameter is illegal). - Fix three assert-then-test contradictions found by the review: the `menu` submenu-keydown guard, and the `chips`/`otp-input` constructors that asserted `findOne(...)!` then tested the result for null. Each now handles the null in a local, so the guard is honest and no `!` remains. The type tests (`api.ts`, `consumer.ts`) now assert consumers cannot reach the protected internals.
1 parent 7a8e251 commit 0f46817

27 files changed

Lines changed: 450 additions & 429 deletions

AGENTS.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,10 @@ Act as a teacher for JavaScript, TypeScript, React, and all JavaScript-family to
5252

5353
- Source is TypeScript (`js/src/**/*.ts`, entry `js/src/index.ts`); ESM-only, no semicolons, 2-space indent
5454
- Imports use `.js` extensions (standard TS ESM style); the build resolves them to `.ts` via `build/rollup-plugin-ts-resolve.cjs`
55-
- Strict tsconfig with `verbatimModuleSyntax` + `erasableSyntaxOnly`; Babel (`@babel/preset-typescript`) strips types, `tsc` only type-checks and emits `.d.ts` (`npm run js-typecheck`, `npm run js-compile-types`)
56-
- Components extend `BaseComponent` (which extends `Config`); per-component config `type XxxConfig` typed off `Default`, refined on the class via `declare _config: XxxConfig`
55+
- Strict tsconfig with `verbatimModuleSyntax` + `erasableSyntaxOnly`; Babel (`@babel/preset-typescript`) strips types, `tsc` only type-checks and emits `.d.ts` (`npm run js-typecheck`, `npm run js-emit-types`)
56+
- Components extend `BaseComponent` (which extends `Config`); per-component config `type XxxConfig` typed off `Default`, refined on the class via `protected declare _config: XxxConfig`
5757
- Instance fields use `declare` (no runtime emit); constructor `element` param stays optional to keep `typeof Component` assignable to `typeof BaseComponent`
58+
- Every `_`-prefixed instance member is `protected` (never `private`, so subclasses keep access); public API methods and statics stay public. This keeps the internals out of the published `.d.ts` surface
5859
- Constants: `NAME`, `DATA_KEY`, `EVENT_KEY`, `VERSION`
5960
- DOM utilities via `dom/event-handler.ts`, `dom/selector-engine.ts`, `dom/manipulator.ts`
6061
- Floating UI for positioning (dropdown, tooltip, popover)

js/src/alert.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class Alert extends BaseComponent {
4747
}
4848

4949
// Private
50-
_destroyElement(): void {
50+
protected _destroyElement(): void {
5151
this._element.remove()
5252
EventHandler.trigger(this._element, EVENT_CLOSED)
5353
this.dispose()

js/src/base-component.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ const VERSION = '6.0.0-alpha1'
2222

2323
class BaseComponent extends Config {
2424
declare ['constructor']: typeof BaseComponent
25-
declare _element: HTMLElement
26-
declare _config: ComponentConfig
25+
protected declare _element: HTMLElement
26+
protected declare _config: ComponentConfig
2727

2828
constructor(element?: string | Element | null, config?: ComponentConfig | null) {
2929
super()
@@ -56,8 +56,8 @@ class BaseComponent extends Config {
5656
}
5757
}
5858

59-
// Private
60-
_queueCallback(callback: () => void, element: Element, isAnimated = true): void {
59+
// Protected
60+
protected _queueCallback(callback: () => void, element: Element, isAnimated = true): void {
6161
executeAfterTransition(() => {
6262
// Don't run the completion callback if the instance was disposed mid-transition
6363
if (!this._element) {
@@ -68,7 +68,7 @@ class BaseComponent extends Config {
6868
}, element, isAnimated)
6969
}
7070

71-
override _getConfig(config?: ComponentConfig | null): ComponentConfig {
71+
protected override _getConfig(config?: ComponentConfig | null): ComponentConfig {
7272
config = this._mergeConfigObj(config, this._element)
7373
config = this._configAfterMerge(config)
7474
this._typeCheckConfig(config)
@@ -80,7 +80,7 @@ class BaseComponent extends Config {
8080
return Data.get(getElement(element), this.DATA_KEY)
8181
}
8282

83-
static getOrCreateInstance<T extends typeof BaseComponent>(this: T, element?: string | Element | null, config: Partial<InstanceType<T>['_config']> | null = {}): InstanceType<T> {
83+
static getOrCreateInstance<T extends typeof BaseComponent>(this: T, element?: string | Element | null, config: NonNullable<ConstructorParameters<T>[1]> | null = {}): InstanceType<T> {
8484
return this.getInstance(element) || (new this(element, typeof config === 'object' ? config : null) as InstanceType<T>)
8585
}
8686

js/src/carousel.ts

Lines changed: 45 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -124,19 +124,19 @@ const easeInOutCubic = (progress: number): number => (progress < 0.5 ?
124124
*/
125125

126126
class Carousel extends BaseComponent {
127-
declare _config: CarouselConfig
128-
declare _viewport: HTMLElement
129-
declare _indicatorsElement: HTMLElement | null
130-
declare _playPauseElement: HTMLElement | null
131-
declare _prevControls: HTMLButtonElement[]
132-
declare _nextControls: HTMLButtonElement[]
133-
declare _interval: ReturnType<typeof setTimeout> | null
134-
declare _observer: IntersectionObserver | null
135-
declare _scrollFrame: number | null
136-
declare _looping: boolean
137-
declare _visibility: Map<Element, number>
138-
declare _playing: boolean
139-
declare _activeIndex: number
127+
protected declare _config: CarouselConfig
128+
protected declare _viewport: HTMLElement
129+
protected declare _indicatorsElement: HTMLElement | null
130+
protected declare _playPauseElement: HTMLElement | null
131+
protected declare _prevControls: HTMLButtonElement[]
132+
protected declare _nextControls: HTMLButtonElement[]
133+
protected declare _interval: ReturnType<typeof setTimeout> | null
134+
protected declare _observer: IntersectionObserver | null
135+
protected declare _scrollFrame: number | null
136+
protected declare _looping: boolean
137+
protected declare _visibility: Map<Element, number>
138+
protected declare _playing: boolean
139+
protected declare _activeIndex: number
140140

141141
constructor(element?: string | Element | null, config?: Partial<CarouselConfig> | null) {
142142
super(element, config)
@@ -308,21 +308,21 @@ class Carousel extends BaseComponent {
308308
// Private
309309
// Normalize an unknown `ends` value so navigation and end-control logic can't
310310
// disagree about whether the carousel wraps.
311-
override _configAfterMerge(config: CarouselConfig): CarouselConfig {
311+
protected override _configAfterMerge(config: CarouselConfig): CarouselConfig {
312312
if (![ENDS_STOP, ENDS_WRAP, ENDS_LOOP].includes(config.ends)) {
313313
config.ends = Default.ends
314314
}
315315

316316
return config
317317
}
318318

319-
_initialActiveIndex(): number {
319+
protected _initialActiveIndex(): number {
320320
const active = SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element)
321321
const index = active ? this._getItems().indexOf(active) : 0
322322
return Math.max(index, 0)
323323
}
324324

325-
_addEventListeners(): void {
325+
protected _addEventListeners(): void {
326326
if (this._config.keyboard) {
327327
EventHandler.on(this._element, EVENT_KEYDOWN, event => this._keydown(event))
328328
}
@@ -336,7 +336,7 @@ class Carousel extends BaseComponent {
336336
EventHandler.on(this._viewport, EVENT_POINTERDOWN, () => this._pauseFromInteraction())
337337
}
338338

339-
_keydown(event: BootstrapEvent): void {
339+
protected _keydown(event: BootstrapEvent): void {
340340
if (/input|textarea/i.test((event.target as HTMLElement).tagName)) {
341341
return
342342
}
@@ -353,7 +353,7 @@ class Carousel extends BaseComponent {
353353
}
354354
}
355355

356-
_observeItems(): void {
356+
protected _observeItems(): void {
357357
// Fade mode stacks slides instead of scrolling, so there's nothing to observe
358358
if (this._isFade() || typeof IntersectionObserver === 'undefined') {
359359
return
@@ -369,7 +369,7 @@ class Carousel extends BaseComponent {
369369
}
370370
}
371371

372-
_handleIntersection(entries: IntersectionObserverEntry[]): void {
372+
protected _handleIntersection(entries: IntersectionObserverEntry[]): void {
373373
// A loop transition deliberately scrolls onto a transient clone; ignore the
374374
// visibility churn so it doesn't move the active index mid-animation.
375375
if (this._looping) {
@@ -412,7 +412,7 @@ class Carousel extends BaseComponent {
412412
// symptom). Fade and non-scrollable layouts have no scroll position to read,
413413
// so they keep using the tracked active index (also what the unit tests rely
414414
// on when there's no real layout).
415-
_navIndex(): number {
415+
protected _navIndex(): number {
416416
if (this._isFade() || (this._viewport.scrollWidth - this._viewport.clientWidth) <= 0) {
417417
return this._activeIndex
418418
}
@@ -432,7 +432,7 @@ class Carousel extends BaseComponent {
432432
return index
433433
}
434434

435-
_scrollToIndex(index: number): void {
435+
protected _scrollToIndex(index: number): void {
436436
const item = this._getItems()[index]
437437
if (!item) {
438438
return
@@ -473,7 +473,7 @@ class Carousel extends BaseComponent {
473473
// jumps overshoot the target and snap back. Because we set every frame's
474474
// absolute position with an instant scroll, the animation can't overshoot and
475475
// every jump takes the same time, in every browser.
476-
_animateScroll(targetLeft: number, onComplete: () => void): void {
476+
protected _animateScroll(targetLeft: number, onComplete: () => void): void {
477477
if (this._scrollFrame !== null) {
478478
cancelAnimationFrame(this._scrollFrame)
479479
this._scrollFrame = null
@@ -521,7 +521,7 @@ class Carousel extends BaseComponent {
521521
// (including the page), so an autoplaying carousel below the fold would yank
522522
// the whole page to itself on each tick. Using bounding rects keeps it
523523
// direction-agnostic (works in RTL).
524-
_scrollDelta(element: HTMLElement): number {
524+
protected _scrollDelta(element: HTMLElement): number {
525525
const viewportRect = this._viewport.getBoundingClientRect()
526526
const rect = element.getBoundingClientRect()
527527

@@ -542,7 +542,7 @@ class Carousel extends BaseComponent {
542542

543543
// Seamless loop: continue past an end into a one-off clone of the destination
544544
// slide, then teleport to the real slide so there's no visible backward jump.
545-
_loopTransition(isNext: boolean): void {
545+
protected _loopTransition(isNext: boolean): void {
546546
const items = this._getItems()
547547
const last = items.length - 1
548548
const fromIndex = this._activeIndex
@@ -608,7 +608,7 @@ class Carousel extends BaseComponent {
608608
})
609609
}
610610

611-
_loopDirection(isNext: boolean): string {
611+
protected _loopDirection(isNext: boolean): string {
612612
if (isRTL()) {
613613
return isNext ? DIRECTION_RIGHT : DIRECTION_LEFT
614614
}
@@ -620,7 +620,7 @@ class Carousel extends BaseComponent {
620620
// viewport during a loop transition. `behavior: 'instant'` is required because
621621
// the viewport sets `scroll-behavior: smooth` in CSS, and `'auto'` would defer
622622
// to it and animate the teleport (a visible backward slide).
623-
_jumpScroll(delta: number): void {
623+
protected _jumpScroll(delta: number): void {
624624
this._viewport.style.scrollSnapType = 'none'
625625
this._viewport.scrollBy({ left: delta, top: 0, behavior: 'instant' })
626626
}
@@ -631,11 +631,11 @@ class Carousel extends BaseComponent {
631631
// mixin). It deliberately avoids the View Transition API: a view transition
632632
// crossfades a page snapshot over its own (shorter) duration while this CSS
633633
// fade also runs underneath, so the two animations overlap and visibly stutter.
634-
_fadeTo(index: number): void {
634+
protected _fadeTo(index: number): void {
635635
this._setActive(index)
636636
}
637637

638-
_setActive(index: number): void {
638+
protected _setActive(index: number): void {
639639
const items = this._getItems()
640640
if (index === this._activeIndex || !items[index]) {
641641
return
@@ -654,7 +654,7 @@ class Carousel extends BaseComponent {
654654
})
655655
}
656656

657-
_refreshActiveState(): void {
657+
protected _refreshActiveState(): void {
658658
const items = this._getItems()
659659

660660
for (const [index, item] of items.entries()) {
@@ -665,7 +665,7 @@ class Carousel extends BaseComponent {
665665
this._updateEndControls()
666666
}
667667

668-
_updateEndControls(): void {
668+
protected _updateEndControls(): void {
669669
// Only `ends: 'stop'` has real ends; under `wrap`/`loop` you can always
670670
// advance, so disabling end controls would be meaningless. When stopping,
671671
// disable the prev control at the start of the scroll range and the next
@@ -700,7 +700,7 @@ class Carousel extends BaseComponent {
700700
this._setControlsDisabled(this._nextControls, atEnd)
701701
}
702702

703-
_setControlsDisabled(controls: HTMLButtonElement[], disabled: boolean): void {
703+
protected _setControlsDisabled(controls: HTMLButtonElement[], disabled: boolean): void {
704704
for (const control of controls) {
705705
// a11y: if we're about to disable the focused control, move focus to the
706706
// opposite (still-enabled) control so focus isn't lost.
@@ -716,7 +716,7 @@ class Carousel extends BaseComponent {
716716
}
717717
}
718718

719-
_setActiveIndicatorElement(index: number): void {
719+
protected _setActiveIndicatorElement(index: number): void {
720720
if (!this._indicatorsElement) {
721721
return
722722
}
@@ -734,7 +734,7 @@ class Carousel extends BaseComponent {
734734
}
735735
}
736736

737-
_normalizeIndex(index: number, length: number): number | null {
737+
protected _normalizeIndex(index: number, length: number): number | null {
738738
if (Number.isNaN(index) || length === 0) {
739739
return null
740740
}
@@ -752,14 +752,14 @@ class Carousel extends BaseComponent {
752752

753753
// Whether navigating past an end wraps to the other end. `loop` continues
754754
// seamlessly where it can (see `_canLoop`) and otherwise behaves like `wrap`.
755-
_wrapsAround(): boolean {
755+
protected _wrapsAround(): boolean {
756756
return this._config.ends === ENDS_WRAP || this._config.ends === ENDS_LOOP
757757
}
758758

759759
// Seamless looping is only supported for the simple single-slide scroll
760760
// layout. Multi-item, peek, center, and variable-width layouts fall back to
761761
// the plain `wrap` jump.
762-
_canLoop(): boolean {
762+
protected _canLoop(): boolean {
763763
if (this._isFade() || this._getItems().length < 2) {
764764
return false
765765
}
@@ -775,7 +775,7 @@ class Carousel extends BaseComponent {
775775
!this._element.classList.contains(CLASS_NAME_AUTO)
776776
}
777777

778-
_direction(from: number, to: number): string {
778+
protected _direction(from: number, to: number): string {
779779
const isNext = to > from
780780
if (isRTL()) {
781781
return isNext ? DIRECTION_RIGHT : DIRECTION_LEFT
@@ -784,7 +784,7 @@ class Carousel extends BaseComponent {
784784
return isNext ? DIRECTION_LEFT : DIRECTION_RIGHT
785785
}
786786

787-
_scheduleAutoplay(index = this._activeIndex): void {
787+
protected _scheduleAutoplay(index = this._activeIndex): void {
788788
const interval = this._itemInterval(index)
789789
// Expose the wait so the active indicator's CSS fill matches it.
790790
this._element.style.setProperty(PROPERTY_INTERVAL, `${interval}ms`)
@@ -810,17 +810,17 @@ class Carousel extends BaseComponent {
810810
// The slide the next autoplay tick will rest on, derived from the live scroll
811811
// position (which still reflects the current slide when the timer fires).
812812
// Returns `null` when there's nowhere left to advance (`ends: stop` at the end).
813-
_upcomingIndex(): number | null {
813+
protected _upcomingIndex(): number | null {
814814
return this._normalizeIndex(this._navIndex() + 1, this._getItems().length)
815815
}
816816

817-
_itemInterval(index = this._activeIndex): number {
817+
protected _itemInterval(index = this._activeIndex): number {
818818
const item = this._getItems()[index]
819819
const interval = item ? Number.parseInt(item.getAttribute('data-bs-interval') as string, 10) : Number.NaN
820820
return Number.isNaN(interval) ? this._config.interval : interval
821821
}
822822

823-
_maybeEnableCycle(): void {
823+
protected _maybeEnableCycle(): void {
824824
if (!this._playing) {
825825
return
826826
}
@@ -846,7 +846,7 @@ class Carousel extends BaseComponent {
846846
this._updatePlayPauseControl()
847847
}
848848

849-
_updatePlayPauseControl(): void {
849+
protected _updatePlayPauseControl(): void {
850850
if (!this._playPauseElement) {
851851
return
852852
}
@@ -862,21 +862,21 @@ class Carousel extends BaseComponent {
862862
}
863863
}
864864

865-
_isFade(): boolean {
865+
protected _isFade(): boolean {
866866
return this._element.classList.contains(CLASS_NAME_FADE)
867867
}
868868

869-
_prefersReducedMotion(): boolean {
869+
protected _prefersReducedMotion(): boolean {
870870
return typeof window !== 'undefined' &&
871871
typeof window.matchMedia === 'function' &&
872872
window.matchMedia('(prefers-reduced-motion: reduce)').matches
873873
}
874874

875-
_getItems(): HTMLElement[] {
875+
protected _getItems(): HTMLElement[] {
876876
return SelectorEngine.find(SELECTOR_ITEM, this._element)
877877
}
878878

879-
_clearInterval(): void {
879+
protected _clearInterval(): void {
880880
if (this._interval) {
881881
clearTimeout(this._interval)
882882
this._interval = null

0 commit comments

Comments
 (0)