ReviewOS
@if (!pullRequest)

No such pull request

stacks/stx has no pull request numbered 2007.

All pull requests stacks/stx
@else
Reload

also looking at this

stacks/stx

fix(components): Button reads disabled and loading as live props

#2007
Merged glennmichael123 wants to merge fix/button-reactive-disabled-loading into main
3 files +255 -25
@if (showWholeFiles)
packages/components/src/ui/button/Button.stxmodified+55-14
@@ -37,10 +37,39 @@ export const spinnerClasses = `animate-spin ${iconClasses}`.trim()
3737
3838<script client>
3939const emit = defineEmits()
40const disabledNow = {{ isDisabled }}
40
41/*
42 * `disabled` and `loading` are read as live props, not as a server snapshot.
43 *
44 * This used to be `const disabledNow = ` — a value computed
45 * before the page was sent and never looked at again. So the most common piece
46 * of button state in an application could not be expressed:
47 *
48 * <Button :disabled="saving()" @click="save">Save</Button>
49 *
50 * `saving()` is a signal. On the server it is not yet meaningful, so the button
51 * shipped enabled and STAYED enabled for the life of the page — through the
52 * exact async action it was there to guard. Double submits, destructive
53 * deletes and rotate-key confirmations all land on this.
54 *
55 * useReactiveProp wires the parent's binding into a local signal: the parent's
56 * bindAttr writes the attribute on our root, a MutationObserver picks it up,
57 * and the signal flips. Dialog already does this for `open` (#1704) and Switch
58 * for `checked`, so this is the package's existing answer rather than a new one.
59 *
60 * See stacksjs/stx#1997.
61 */
62const isLoading = useReactiveProp('loading', {{ loading }})
63const disabledProp = useReactiveProp('disabled', {{ disabled }})
64
65// Loading implies disabled, the same combination the server computes for the
66// initial render, so the two agree before and after hydration.
67const isDisabled = derived(() => disabledProp() || isLoading())
4168
4269function onClick(event) {
43 if (disabledNow) {
70 // Read through the signal: a button disabled after render must stop guarding
71 // on the live value, not on what was true when the page was built.
72 if (isDisabled()) {
4473 event.preventDefault()
4574 return
4675 }
@@ -52,23 +81,35 @@ function onClick(event) {
5281 type=""
5382 class=""
5483 @if(isDisabled)disabled@endif
55 @click="onClick($event)"
84 :disabled="isDisabled()"
5685 aria-busy="false"
86 :aria-busy="isLoading() ? 'true' : 'false'"
87 @click="onClick($event)"
5788>
58 @if(loading)
59 <svg class="" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24">
60 <circle class="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" stroke-width="4"></circle>
61 <path class="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z"></path>
62 </svg>
63 @else
64 @if(leftIcon)
65 <span class="">{!! leftIcon !!}</span>
66 @endif
89 {{-- The spinner is always in the DOM so `loading` can turn it on after render;
90 it was inside an @if, which meant the only way to see it was to have been
91 loading when the page was built. x-cloak only when the server says we are
92 NOT loading, so a button rendered loading shows its spinner on first
93 paint rather than waiting for hydration. --}}
94 <svg
95 :show="isLoading()"
96 @if(!loading)x-cloak@endif
97 class=""
98 xmlns="http://www.w3.org/2000/svg"
99 fill="none"
100 viewBox="0 0 24 24"
101 >
102 <circle class="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" stroke-width="4"></circle>
103 <path class="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z"></path>
104 </svg>
105
106 @if(leftIcon)
107 <span :show="!isLoading()" @if(loading)x-cloak@endif class="">{!! leftIcon !!}</span>
67108 @endif
68109
69110 <slot />
70111
71 @if(!loading && rightIcon)
72 <span class="">{!! rightIcon !!}</span>
112 @if(rightIcon)
113 <span :show="!isLoading()" @if(loading)x-cloak@endif class="">{!! rightIcon !!}</span>
73114 @endif
74115</button>
packages/components/test/button-reactive-state.test.tsadded+144-0
@@ -0,0 +1,144 @@
1/**
2 * Button's `disabled` and `loading` follow the parent's signal after render.
3 *
4 * Both props used to be evaluated once on the server and frozen:
5 *
6 * const disabledNow =
7 * <button >
8 *
9 * so the single most common piece of button state in an application could not
10 * be expressed at all:
11 *
12 * <Button :disabled="saving()" @click="save">Save</Button>
13 *
14 * `saving()` is a signal. On the server it is not yet meaningful, `isDisabled`
15 * resolved false, and the button shipped enabled and stayed enabled for the
16 * life of the page — through the exact async action it was there to guard.
17 * Double-submit protection, destructive deletes and rotate-key confirmations
18 * all land on this. The same freeze hid the built-in spinner: `loading` could
19 * only ever be true if it had been true when the page was built, which is
20 * never the case for an action that starts on a click.
21 *
22 * The reporting app measured 22 of its 50 otherwise-migratable buttons as
23 * depending on a reactive `:disabled`, and kept a plain `<button>` for them.
24 *
25 * stacksjs/stx#1997.
26 */
27import { readFileSync } from 'node:fs'
28import path from 'node:path'
29import { afterEach, describe, expect, it } from 'bun:test'
30import { boot, closeBrowser, find, layout, page, renderApp, settle } from '../../stx/test/router/spa-harness'
31
32const UI = path.join(import.meta.dir, '..', 'src', 'ui')
33
34/** The shipped component, not a copy: the point is to test what apps install. */
35function component(rel: string): string {
36 return readFileSync(path.join(UI, rel), 'utf-8')
37}
38
39/**
40 * A parent that owns the signal, which is the shape the bug is about: the value
41 * is not known until something happens on the client, long after render.
42 */
43const HOST = `<script client>
44 const saving = state(false)
45 function flip() { saving.set(!saving()) }
46</script>
47
48<div id="host">
49 <Button className="probe" :disabled="saving()" :loading="saving()">Save</Button>
50</div>`
51
52const FILES = {
53 'layouts/app.stx': layout(''),
54 'components/Button.stx': component('button/Button.stx'),
55 'components/Host.stx': HOST,
56 'pages/index.stx': page('app', '<Host />'),
57}
58
59const ROUTES = { '/': 'pages/index.stx' }
60
61interface Harness {
62 /** The component's own <button>, found by the className the parent passed. */
63 button: () => any
64 /** The spinner, which stays in the DOM and is shown by `:show`. */
65 spinner: () => any
66 /** Flip the parent's signal. */
67 flip: () => void
68 dispose: () => Promise<void>
69}
70
71async function mount(): Promise<Harness> {
72 const app = await renderApp(FILES, ROUTES)
73 const browser = await boot(app, '/')
74 await settle()
75 const hostEl = find(browser, '#host').closest('[data-stx-scope]')
76 const hostScope = browser.window.stx._scopes[hostEl.getAttribute('data-stx-scope')]
77 return {
78 button: () => find(browser, 'button.probe'),
79 spinner: () => find(browser, 'button.probe svg'),
80 // Called on the scope rather than dispatched: this harness renders the
81 // @click attribute but does not wire it as a DOM listener, the same reason
82 // dialog-modal-behaviour.test.ts invokes its handler directly.
83 flip: () => hostScope.flip(),
84 dispose: () => app.dispose(),
85 }
86}
87
88/** `:show` toggles display rather than detaching, so ask the style. */
89function visible(el: any): boolean {
90 return !!el && el.style.display !== 'none'
91}
92
93afterEach(() => {
94 closeBrowser()
95})
96
97describe('#1997 — Button reads disabled and loading as live props', () => {
98 it('ships enabled while the parent signal is false', async () => {
99 const h = await mount()
100 expect(h.button().hasAttribute('disabled')).toBe(false)
101 expect(h.button().getAttribute('aria-busy')).toBe('false')
102 await h.dispose()
103 })
104
105 it('disables when the parent signal flips after render', async () => {
106 const h = await mount()
107 h.flip()
108 await settle()
109 // The whole bug: this stayed false forever, because the attribute came from
110 // a value computed before the page was sent.
111 expect(h.button().hasAttribute('disabled')).toBe(true)
112 await h.dispose()
113 })
114
115 it('re-enables when the signal flips back, so the state is not one-way', async () => {
116 const h = await mount()
117 h.flip()
118 await settle()
119 h.flip()
120 await settle()
121 expect(h.button().hasAttribute('disabled')).toBe(false)
122 await h.dispose()
123 })
124
125 it('marks itself busy for assistive tech while loading', async () => {
126 const h = await mount()
127 h.flip()
128 await settle()
129 expect(h.button().getAttribute('aria-busy')).toBe('true')
130 await h.dispose()
131 })
132
133 it('reveals the spinner when loading turns on after render', async () => {
134 const h = await mount()
135 // Present from the start but hidden. It used to live inside an @if, so the
136 // only way to see it was to have been loading when the page was built.
137 expect(h.spinner()).toBeTruthy()
138 expect(visible(h.spinner())).toBe(false)
139 h.flip()
140 await settle()
141 expect(visible(h.spinner())).toBe(true)
142 await h.dispose()
143 })
144})
packages/components/test/visual/__snapshots__/button-template.snap.txtmodified+56-11
@@ -42,10 +42,42 @@ export const spinnerClasses = `animate-spin ${iconClasses}`.trim()
4242</script>
4343<script client>
4444 const emit = defineEmits()
45const disabledNow =
45
46/*
47 * `disabled` and `loading` are read as live props, not as a server snapshot.
48 *
49 * This used to be `const disabledNow = ` — a value computed
50 * before the page was sent and never looked at again. So the most common piece
51 * of button state in an application could not be expressed:
52 *
53 *
54 <Button :disabled="saving()" @click="save">
55 Save
56 </Button>
57 *
58 * `saving()` is a signal. On the server it is not yet meaningful, so the button
59 * shipped enabled and STAYED enabled for the life of the page — through the
60 * exact async action it was there to guard. Double submits, destructive
61 * deletes and rotate-key confirmations all land on this.
62 *
63 * useReactiveProp wires the parent's binding into a local signal: the parent's
64 * bindAttr writes the attribute on our root, a MutationObserver picks it up,
65 * and the signal flips. Dialog already does this for `open` (#1704) and Switch
66 * for `checked`, so this is the package's existing answer rather than a new one.
67 *
68 * See stacksjs/stx#1997.
69 */
70const isLoading = useReactiveProp('loading', )
71const disabledProp = useReactiveProp('disabled', )
72
73// Loading implies disabled, the same combination the server computes for the
74// initial render, so the two agree before and after hydration.
75const isDisabled = derived(() => disabledProp() || isLoading())
4676
4777function onClick(event) {
48 if (disabledNow) {
78 // Read through the signal: a button disabled after render must stop guarding
79 // on the live value, not on what was true when the page was built.
80 if (isDisabled()) {
4981 event.preventDefault()
5082 return
5183 }
@@ -56,26 +88,37 @@ function onClick(event) {
5688 type=""
5789 class=""
5890
59 @click="onClick($event)"
91 :disabled="isDisabled()"
6092 aria-busy="false"
93 :aria-busy="isLoading() ? 'true' : 'false'"
94 @click="onClick($event)"
6195>
62
70
76119 <slot />
77