Repository navigation
refactor(core): one shared touch module that components import directly, reached by Lab through internal/touch - #8465
Conversation
A build that defines __ASTRYX_TOUCH__ as false presents every component the pointer way and leaves out each touch layer imported through the guard touchLayers.ts documents. Without the define nothing changes. The browser support guide covers the build settings, device routing and the Next.js caveat.
|
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
…aved draft The section's last sentence ended on "saves", which matches "saved" in the main searcher's "toast notice for a saved draft" query. The section took the 20th result from a shorter doc and grew that answer past the lexical ranker's, which the main searcher's byte check rejects. The sentence now ends on "is worth"; a guard test pins that the guide is found for its own topic and not for that query.
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner: no current spec provides a build-time opt-out from touch presentation (spec:AST-094 FR13 and DEC-1 apply touch presentation under every coarse pointer, and architecture:component-style-authoring INV6 gives touch-only JavaScript only lazy loading or a justification), so an engineering owner needs to provide or identify the governing spec for __ASTRYX_TOUCH__ and the import-site guard, recording that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Whatever that decision, these need fixing:
- The guide and the changeset say a bundle built with
__ASTRYX_TOUCH__asfalsepresents every component the pointer way and leaves touch code out, but nothing reads the resolver yet: Selector, MultiSelector, DropdownMenu and ContextMenu decide throughuseAdaptivePresentation, the date and time inputs read(pointer: coarse)themselves, and no import site writes the guard. - The new guide section makes
packages/cli/api/search/_main-searcher.test.mjsfail ('never returns more results or more bytes than the lexical ranker', 67347 > 67269), astest-node (1/4)reports. - The guide's
User-Agentrouting sends iPads the desktop bundle, because iPadOS Safari reports a macOSUser-Agentby default.
[Automated review]
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: "One Astryx bundle serves every device, so it carries the code components use under a finger: gesture handling, springs and each component's touch behavior. A desktop never runs that code. If you already serve desktops a bundle of their own, build it with `__ASTRYX_TOUCH__` defined as `false`. In that bundle every component presents the pointer way on any device and leaves its touch code out. A build without the define keeps it, as before.", |
There was a problem hiding this comment.
This doesn't hold at this head: nothing outside touchPresentation.ts calls useTouchPresentation or readTouchPresentation, and no import site writes the guard. A bundle built with __ASTRYX_TOUCH__ as false still presents touch on a phone (Selector, MultiSelector, DropdownMenu and ContextMenu resolve adaptive through useAdaptivePresentation; DateInput, DateTimeInput and TimeInput read (pointer: coarse) themselves) and keeps every touch module. Either describe what this release does, or route those decisions through the resolver here.
| }, | ||
| { | ||
| type: 'prose', | ||
| text: 'Serve the desktop bundle only to devices whose primary pointer is a mouse or trackpad, and the full bundle to phones and tablets. Choose on the server from the `User-Agent` header: the `Sec-CH-UA-Mobile` client hint comes only from Chromium-based browsers, so it cannot pick out an iPhone. A device sent the desktop bundle by mistake still works the pointer way; the stylesheet is the same in both bundles, so its touch sizing still applies.', |
There was a problem hiding this comment.
iPadOS Safari requests desktop websites by default and then sends a macOS User-Agent, so routing on User-Agent serves iPads the desktop bundle. Worth saying here, with what to do instead (serve the full bundle when unsure, or choose on the client).
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] A bundle can leave out Astryx's touch code. Build it with `__ASTRYX_TOUCH__` defined as `false` (esbuild or Vite `define`, webpack `DefinePlugin`, Next.js `compiler.define`) and every component presents the pointer way on any device, so a product that serves desktops a bundle of their own stops shipping touch behavior there. Touch modules imported through the documented guard drop out of that bundle. Without the define nothing changes. The browser support guide (`astryx docs browser-support`) shows the build settings, how to route each device to its bundle, and the Next.js caveat. |
There was a problem hiding this comment.
Same as the guide: at this head no component reads the flag and no touch module is imported through the guard, so 'every component presents the pointer way on any device' and 'Touch modules imported through the documented guard drop out of that bundle' don't describe this release yet.
| const compact = useCompactDefinition(); | ||
| const isTouch = useMediaQuery(TOUCH_QUERY, serverDefault.isTouch ?? false); | ||
| const isCoarse = useMediaQuery(TOUCH_QUERY, serverDefault.isTouch ?? false); | ||
| const isTouch = TOUCH_LAYERS && isCoarse; |
There was a problem hiding this comment.
The gate itself is right, but nothing in core or lab calls useTouchPresentation or readTouchPresentation outside tests, so the flag changes no component yet. useAdaptivePresentation (hooks/useAdaptivePresentation.ts:34-36) and the date and time inputs' own (pointer: coarse) reads would have to go through here for the guide's promise to hold.
PR Analysis Report
View Storybook for this PR · View Sandbox for this PR Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
Approval waits on an engineering owner: no current spec provides a build-time opt-out from touch presentation (spec:AST-094 FR13 and DEC-1 apply touch presentation under every coarse pointer, and architecture:component-style-authoring INV6 gives touch-only JavaScript only lazy loading or a justification), so an engineering owner needs to provide or identify the governing spec for __ASTRYX_TOUCH__ and the import-site guard, recording that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. The search-ranker failure from the last round is fixed, but these still need fixing whatever that decision:
- The guide, the changeset and the
touchLayers.tsheader say a bundle built with__ASTRYX_TOUCH__asfalsepresents every component the pointer way and leaves touch code out, yet no component reads the gated resolver (Selector, MultiSelector, DropdownMenu and ContextMenu decide throughuseAdaptivePresentation, and the date and time inputs read(pointer: coarse)themselves) and no import site writes the guard. - The guide's
User-Agentrouting sends iPads the desktop bundle, because iPadOS Safari requests desktop sites by default and reports a macOSUser-Agent.
[Automated review]
…ind it through one internal hook @astryxdesign/core/touch-support exports TouchSupport: install() (idempotent, a no-op on the server, adds no listeners) and isInstalled(), nothing else. The module (main's gesture core; the animator joins it) sits in one module-level registry that components read only through the internal useTouchSupport hook, null on the server and until installed. Lint keeps the internals to Core and Lab, and the exports map holds only the public entry. A bundle test pins that core's root, hooks and utils entries and a component reading the hook carry none of the module, and that an install carries it. Replaces this branch's __ASTRYX_TOUCH__ define, resolver change and guide section, which the install makes unnecessary.
vjeux
changed the title
CI-Rerun: one RTL story navigation timed out in pr-rtl (core 2/3) of run 40273227303; it does not reproduce at this tree.
There was a problem hiding this comment.
Approval waits on an engineering owner to provide or identify the governing spec for the new @astryxdesign/core/touch-support entry: architecture:react-component-runtime INV13, which it builds on, is still proposed in #8464. That decision also has to settle the guard against import() of the touch internals, which current architecture:component-style-authoring INV6 and spec:AST-080 DEC-4 still allow as lazy loading on a coarse pointer, and the app-level install that spec:AST-094 DEC-1 ("without an opt-in") rules out once components read the hook; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, please make the TouchSupport JSDoc, which ships in the declaration file, describe this release: it promises an animator, touch behavior in components, Reduce Motion handling, a useAnimatedLayout hook that does not exist, and nothing for mouse-only visitors, while no component reads the module yet and install() imports and registers it (about 5.7 KB gzip) on every pointer.
[Automated review]
| * Touch support for Astryx components: the gesture core and the animator, | ||
| * installed once for the whole page. | ||
| * | ||
| * Call `install()` once in your client entry, before you render or hydrate: | ||
| * | ||
| * ``` | ||
| * import TouchSupport from '@astryxdesign/core/touch-support'; | ||
| * | ||
| * TouchSupport.install(); | ||
| * ``` | ||
| * | ||
| * - Components with touch behavior use it under a finger. Without it they | ||
| * keep their plain behavior and stay fully usable, and a tap still acts as | ||
| * a click. | ||
| * - Motion such as `useAnimatedLayout` works without it. | ||
| * - People using only a mouse pay nothing: on a fine pointer the module | ||
| * registers nothing. | ||
| * - A swipe-only action still needs a non-touch path, such as a button or a | ||
| * menu item, for a mouse, a keyboard, or a page without touch support. | ||
| * - Reduce Motion is respected. |
There was a problem hiding this comment.
This text ships as the editor hover in dist/touch-support/index.d.ts, but at this head the module holds no animator (touchModule.ts:41-58), no component reads it, useAnimatedLayout does not exist in Astryx, nothing reads a motion preference, and install() imports and registers the module on every pointer, so a page that installs it sends about 5.7 KB gzip to mouse-only visitors too. Could it describe this release instead: it registers the gesture core for components to use in later releases, and no component uses it yet?
There was a problem hiding this comment.
The doc now describes this release (3082b98): the gesture core, which no component uses yet, and its code ships with the entry. The animator, component behavior and Reduce Motion lines are gone until those releases.
There was a problem hiding this comment.
Approval waits on an engineering owner to update the existing spec for the new @astryxdesign/core/internal/touch subpath: on main, spec:AST-017 FR21 says a gesture module is "never exported, as experimental or otherwise" and architecture:public-component-api INV12 treats anything reachable from a package path as public, while the bundle test's ban on import() of the touch modules closes the lazy-loading path that architecture:component-style-authoring INV6 and spec:AST-080 DEC-4 still allow. Those amendments, including architecture:react-component-runtime INV13, are approved in #8464 but not yet on main, so landing #8464 first, or an engineering owner commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner), settles it. Two wording fixes are needed either way:
- The rule's README and header say it covers every file outside Core and Lab, but
.mjsfiles outside the CLI,scripts/and.github/scripts/are never linted, so "every linted file" would be accurate. - The changeset says Core and Lab components share the module, though none imports it yet.
[Automated review]
| `export … from`, `import()`, `require()` and `typeof import()`. The repo config | ||
| turns it on for every file outside `packages/core` and `packages/lab`, so only | ||
| Core and Lab may import them. |
There was a problem hiding this comment.
The global ignores in eslint.config.js (lines 141-166) never lint scripts/**, .github/scripts/** or any .mjs outside seven packages/cli directories, so an @astryxdesign/core/internal/touch import in, say, apps/docsite/probe.mjs passes. "Every linted file outside packages/core and packages/lab" would match what the config does; the rule header (no-core-internal-import.js:5-8) says the same and needs the same change.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Core and Lab components share one touch and gesture module through an internal entry reserved for Astryx's own packages. Apps import nothing new. |
There was a problem hiding this comment.
No Core or Lab component imports touchModule at this head, so "Core and Lab components share one touch and gesture module" describes a later release. Saying the module is added for Core and Lab components to share would match what ships.
No Core or Lab component imports touchModule yet, so the changeset says the module is added for them to share. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…h module's gathering
There was a problem hiding this comment.
Approval waits on an engineering owner to update the existing spec for the new @astryxdesign/core/internal/touch subpath: on main, spec:AST-017 FR21 says a gesture module is "never exported, as experimental or otherwise" and architecture:public-component-api INV12 treats anything reachable from a package path as public, while the bundle test's ban on import() of the touch modules closes the lazy-loading path that architecture:component-style-authoring INV6 and spec:AST-080 DEC-4 still allow. Those amendments, including architecture:react-component-runtime INV13, are approved in #8464 but not yet on main, so landing #8464 first, or an engineering owner commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner), settles it. One wording fix is needed either way: the rule's README and header say it covers every file outside packages/core and packages/lab, but scripts/, .github/scripts/ and .mjs files outside the CLI are never linted, so "every linted file" would be accurate.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner to update the existing spec for the new @astryxdesign/core/internal/touch subpath: on main, spec:AST-017 FR21 says a gesture module is "never exported, as experimental or otherwise" and architecture:public-component-api INV12 treats anything reachable from a package path as public, while the bundle test's ban on import() of the touch modules closes the lazy-loading path that architecture:component-style-authoring INV6 and spec:AST-080 DEC-4 still allow. Those amendments, including architecture:react-component-runtime INV13, are approved in #8464 but not yet on main, so landing #8464 first, or an engineering owner commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner), settles it. The lint-coverage wording from the previous round is fixed, and nothing else needs to change once #8464 is on main.
[Automated review]
…ith no gathering object architecture:component-style-authoring INV6 (UIKit tables ship only the rows a component uses) and the maintainer's direction that internal modules are imported statically, each with one job, with no grab-bag object: a frozen touchModule made every adopter carry every member it held, and the rows they read. internal/touch now re-exports the gesture core's primitives by name, so a bundler keeps one copy per page and only what a component uses; touch-support/touchModule.ts goes. The entry test moves to internal/touch.test.ts and checks the named re-exports, no gathering object and no listeners on load; the bundle test imports GestureArena by name. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
… useTouchLayer Rebuilt on meta#8465's direction (architecture:component-style-authoring INV6; each module one job, imported statically): there is no touchModule object, so this PR drops its gathering too and keeps only useTouchLayer and its registry. A layer's setup gets the element alone and imports the touch modules it uses itself; the registry answers whether a layer runs now (isTouchLayerActive: the primary pointer is coarse), with resetTouchSupportForTests kept as the internal test reset. The pointer read, the Activity re-read and the fine-pointer tests are unchanged. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…by name) as a dependency meta#8465 dropped the frozen `touchModule` object. internal/touch keeps this pull request's named additions (the pan recognizer, the gesture types and the touch queries) beside meta#8465's gesture-core exports, with no gathering object; the entries' lint test takes meta#8465's severity check and no longer lists the removed module, and touch.test.ts checks the added exports by name. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
There was a problem hiding this comment.
Approval waits on an engineering owner to update the existing spec for the new @astryxdesign/core/internal/touch subpath: on main, spec:AST-017 FR21 says a gesture module is “never exported, as experimental or otherwise”, architecture:public-component-api INV12 treats anything reachable from a package path as public, and architecture:component-style-authoring INV6 and spec:AST-080 DEC-4 still allow the lazy import() path that the bundle test now fails. The amendments this change relies on, including architecture:react-component-runtime INV13, are in #8464, so landing #8464 on main first, or an engineering owner commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner), settles it. Separately, please update the description, which still describes the removed frozen touchModule and a touchModule.test.ts, and drop the core/src/touch-support references from the lint rule, its README and eslint.config.js, since that directory no longer exists.
[Automated review]
| // their own: by package path, or by a path into Core's source. | ||
| const DEEP = [ | ||
| /^@astryxdesign\/core\/(?:gestures|motion)(?:\/|$)/, | ||
| /(?:^|\/)core\/src\/(?:gestures|motion|touch-support)(?:\/|$)/, |
There was a problem hiding this comment.
core/src/touch-support exists neither at this head nor on main: the latest commit removed touchModule.ts along with that directory. This alternative, the deep message on line 45, the README's {gestures,motion,touch-support} and the case at no-core-internal-import.test.mjs:106 still describe it, so a contributor who hits the message is pointed at source that is gone. Dropping touch-support from the pattern, message, README and case, or noting that it is kept for a later module, keeps them accurate.
There was a problem hiding this comment.
This looks ready to merge: the new @astryxdesign/core/internal/touch entry and its tests match the touch-module and internal-subpath rules now on main (architecture:react-component-runtime INV13, with the ./internal/* provisions in spec:AST-017 FR21 and FR25 and architecture:public-component-api INV12), and the tests catch a load-time listener, a Lab import() and gesture code leaking into Core's public entries. Nothing else is required; if you like, trim the description's Change and Testing sections, which still describe the removed touchModule object, before squashing.
[Automated review]
|
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. |
…Core's internal entries Main's internal/touch, its test, the touch-support bundle test, the internal-entries test, sync-exports and the exports map are kept exactly; this branch adds only what main lacks: internal/motion, the named re-exports Lab builds on (the pan recognizer, the gesture types, the touch presentation queries), their tests and the COVERED list. # Conflicts: # packages/core/package.json # packages/core/src/internal/touch.test.ts # packages/core/src/internal/touch.ts # scripts/core-internal-entries.test.mjs # scripts/sync-exports.js # scripts/touch-support-bundle.test.mjs
…nothing else does (#8671) ## Why Every pull request that gives a component a gesture fails `test-node` once merged with main, and the failure names no component: `scripts/touch-support-bundle.test.mjs` requires 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-runtime` INV13), and the root entry re-exports every component, so it carries gesture code as soon as one component does. ## Options and tradeoffs - Exempt the root entry: unblocks everyone, but stops catching a component that pulls gesture code it does not use. - Check each public import on its own: catches exactly that, at the cost of a list of the imports allowed to carry gesture code. ## 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 - Each public import of Core is built the way the size report builds it, and may carry gesture code only if `GESTURE_USERS` names it. A failure lists the import, its gesture modules with their sizes, and the chain of imports that brings them. - The root entry may carry only what a named import carries; the utils entry stays gesture-free. - A strict case keeps Button and TextInput free of gesture code whatever the list says. - `GESTURE_USERS` starts 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 1. Run `pnpm exec vitest run --project node scripts/touch-support-bundle.test.mjs` on main with this branch merged: it passes. 2. On a tree whose component uses a gesture, run it without naming that import in `GESTURE_USERS`: the test fails and names that import and the chain that brings the gesture code. Name it, and the test passes. 3. Import a gesture module from Button: the strict case fails. ## Evidence - With the list empty, the test passes on main: no public import carries gesture code today. - On open trees that give a component a gesture (AppShell's scroll-edge bar, Slider, the List reorder), the per-import case fails naming exactly those imports and their chains, and passes once each is named. - No product code changes. ## Scope - [x] No intended public API, product behavior, visual direction, or policy change. - [x] Product changes discovered during the work were removed or split. - [x] Public text and artifacts contain no internal Meta context.
Review only this PR's change: vjeux/astryx@12e7190...7fa9150. This push merges main with #8561.
Update (head 3ca280f): there is no gathering object.
internal/touchre-exports the gesture core (the arena, the recognizers, their geometry) by name, so a bundler keeps one copy per page and only what a component uses;touch-support/touchModule.tsis gone. The shared@astryx/no-core-internal-importrule still namescore/src/touch-support: #8518, stacked on this PR, adds that directory back (useTouchLayer and its registry), so the rule is left as is. Text below that mentionstouchModulepredates this.Maintainer impact
Touch-capable components need one gesture core, and Lab components need a supported way to reach it. Without one, each component that adds touch behavior would bring its own copy, and Lab would reach into Core's source paths.
Intended invariant
No public API or product change. Per
architecture:react-component-runtimeINV13 as #8464 now records it: the touch and gesture code is one set of internal modules that components import directly, one copy per page, with no install step. Under a mouse or another fine pointer nothing attaches listeners (spec:AST-094FR19, FR24). Lab reaches the modules through Core's internal subpaths.Change
touch-support/touchModule.tsgathers the gesture core (the arena, the recognizers, their geometry) in one frozen object. Core components import it by relative path.@astryxdesign/core/internal/touchexportstouchModuleand its type for Lab: undocumented, outside the public API, and allowed by lint only in Core and Lab, sinceno-restricted-importsrefuses@astryxdesign/core/internal/*elsewhere, beside the gesture and motion source paths.scripts/sync-exports.jslists it with the other file subpaths.TouchSupport.install(),isInstalled(), registry anduseTouchSupportare removed.[chore]changeset, since the exports map ships the internal entry.Evidence
internal/touchgets the module, and two such components share one copy.Scope
Testing
scripts/touch-support-bundle.test.mjs: Core's root, hooks and utils entries carry none of the module; two components importing it share one copy in a split bundle (with splitting off, the test fails); the exports map has no entry for it besideinternal/touch; no consumer doc orimport()reaches it.touchModule.test.ts: the module gathers the gesture core, frozen; the internal entry exposes that same object; it loads without adding a window or document listener (a listener added at load fails the test).scripts/core-internal-entries.test.mjs: lint refusesinternal/touchandinternal/reorderoutside Core and Lab and allows them inside.CI=true pnpm lint,verify-exports,sync:exports:check,check:changesets.