Repository navigation
test(core): a touch component's import carries its gesture code, and nothing else does - #8671
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested for one mismatch between the description and the code; the per-import check itself works as intended. A listed component's static import of the touch modules now passes, and an unlisted import that starts carrying gesture code fails with its name, modules, bytes and import chain. The description says GESTURE_USERS is empty and main stays as strict as today, but the head lists 25 imports that carry no gesture code on main, so, for example, a gesture import added to Pagination passes this test while main's version fails it. Please either start the list empty and let each touch pull request name its own import, or update the Change and Testing sections to list these entries, the open work each one is for, and the new Button/TextInput case.
[Automated review]
| const GESTURE_USERS = new Set([ | ||
| // AppShell's scroll-edge bar. | ||
| 'AppShell', | ||
| // Slider's thumb. | ||
| 'Slider', | ||
| // The List reorder, through createReorderable. | ||
| 'reorderable', | ||
| // DateInput's and TimeInput's wheel, and the imports that reach it. | ||
| 'DateInput', | ||
| 'DateTimeInput', | ||
| 'PowerSearch', | ||
| 'Table', | ||
| 'TimeInput', | ||
| // useMenuPress's trigger, which waits on a scroll view under a finger. | ||
| 'hooks: useMenuPress', | ||
| // useScrollLock's page hold, and the layered components that reach it. | ||
| 'hooks: useScrollLock', | ||
| 'Layer', | ||
| 'AlertDialog', | ||
| 'BottomSheet', | ||
| 'CommandPalette', | ||
| 'ContextMenu', | ||
| 'Dialog', | ||
| 'Drawer', | ||
| 'DropdownMenu', | ||
| 'Lightbox', | ||
| 'Markdown', | ||
| 'MoreMenu', | ||
| 'MultiSelector', | ||
| 'Pagination', | ||
| 'Popover', | ||
| 'Selector', | ||
| ]); |
There was a problem hiding this comment.
These 25 entries are allowances on main today: none of these imports carries gesture code yet (reorderable isn't a public import), so any of them can start carrying it without an edit here. For example, re-exporting GestureArena from Pagination/index.ts passes this file, while main's version fails it on the root entry. The PR description says this list is empty. Could you either start it empty and let each touch PR add its own import in review, or update the description to list these entries and the open work each one is for?
| * in it, built as the size report builds a component import. React and StyleX | ||
| * stay external: they hold no touch code. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
Nit: the comment just above (lines 39-43) still documents outputsOf, which now sits at line 69 without one. Consider moving it back down.
…at among the gesture users) into the keyboard dismiss
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
…public pan entries Main's guard holds the root and hooks entries to no gesture code at all, which the public pan (createPanGesture, usePanGesture) cannot meet by design. meta#8671's guard checks each public import and lists usePanGesture (which the root's createPanGesture shares) as a gesture user, so the bundle test passes here as it does on meta#8671's own CI. Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…extMenu's entries Main's guard holds the root entry to no gesture code at all, which ContextMenu's long press cannot meet once the root re-exports it. meta#8671's guard checks each public import and lists ContextMenu as a gesture user, so the root carries only what its touch components import. Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…drag-to-snap-points entries Main's guard holds the hooks entry to no gesture code at all, which useDragToSnapPoints cannot meet by design. meta#8671's guard checks each public import and lists it (and usePanGesture) as gesture users. meta#8655 takes the same merge; this PR does not contain meta#8655's current head, so it takes meta#8671 directly rather than meta#8655's unrelated changes. Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…ndle guard, and pressableCoverage's model files)
…nothing else does scripts/touch-support-bundle.test.mjs required Core's root and hooks entries to carry none of the gesture code. Under architecture:react-component-runtime INV13 a component that uses a gesture imports the touch module statically, so the root, which re-exports every component, carries it as soon as one does. The claim that holds is per import: each public import of Core (each subpath export but the root and the internal entries, and each hook on its own), built as the size report builds one, carries gesture code only if it is named in GESTURE_USERS, and the root entry carries only gesture modules a named import carries. A failure lists the import, its gesture modules with their bytes, and the import chain that brings them. The utils entry stays gesture-free as a whole; the other cases are unchanged. GESTURE_USERS is empty, so main is held to what it is held to today. On four open trees (AppShell's scroll-edge bar, Slider, the List reorder, and RefreshControl's page hold through the layer runtime) the previous root case fails; the new cases fail naming exactly those imports with their chains, and pass once each is named. Rebased onto main as one change. GESTURE_USERS starts empty: each pull request that gives an import a gesture adds that import. Folds in the follow-ups: - test(core): the imports open work makes gesture users are listed; Button and TextInput never carry gesture code - test(core): useMenuPress may carry the native-scroll probe - test(core): the imports that open touch pull requests make gesture users may carry gesture code
de288b0 to
d857999
Compare
There was a problem hiding this comment.
Recommending approval: the per-import check works as described, and the description now matches the code, with GESTURE_USERS empty so every import stays as strict as on main. A component that starts using a gesture fails by name, with its gesture modules, their sizes and the import chain, and passes once its import is listed, while Button and TextInput fail whatever the list says. No changes are needed here; each touch pull request now adds its own imports to GESTURE_USERS.
[Automated review]
cixzhang
added
the
ci:run
Why
Every pull request that gives a component a gesture fails
test-nodeonce merged with main, and the failure names no component:scripts/touch-support-bundle.test.mjsrequires Core's root and hooks entries to carry no gesture code. At least six open pull requests (#8142, #8190, #8337, #8159, #8098, #8658) fail it this way.What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do. Since #8465, a component that uses a gesture imports the touch modules directly (
architecture:react-component-runtimeINV13), and the root entry re-exports every component, so it carries gesture code as soon as one component does.Options and tradeoffs
Why this choice
The per-import check keeps INV13's promise, a component without gestures ships none of their code, while letting gesture users through by name. It was offered on #8562 and aimed at main's test.
What changes
GESTURE_USERSnames it. A failure lists the import, its gesture modules with their sizes, and the chain of imports that brings them.GESTURE_USERSstarts empty, so every import stays as strict as on main. The pull request that gives a component a gesture adds that component's import, and each import that reaches it, with a comment naming the gesture, so the new gesture user shows up in review.How to check it
pnpm exec vitest run --project node scripts/touch-support-bundle.test.mjson main with this branch merged: it passes.GESTURE_USERS: the test fails and names that import and the chain that brings the gesture code. Name it, and the test passes.Evidence
Scope