diff --git a/BREAKING.md b/BREAKING.md index 19194cc658b..2ca32ea114b 100644 --- a/BREAKING.md +++ b/BREAKING.md @@ -466,6 +466,32 @@ The `@ionic/react-router` package now requires React Router v6. React Router v5 | react-router | 6.4.0+ | | react-router-dom | 6.4.0+ | +**TypeScript** + +The `@ionic/react` package now requires TypeScript 5.4 or later. Its type definitions use `NoInfer`, which TypeScript added in 5.4. This matches the minimum that `@ionic/angular` already requires. + +**Typed Overlay Hook Props** + +The `useIonModal` and `useIonPopover` hooks type `componentProps` against the component they are given, instead of accepting `any`. Props that do not match the component are a compile error, and `componentProps` is required when the component declares required props. Applications passing incorrect props will see new type errors at build time rather than failing at runtime. + +```diff + const Modal: React.FC<{ title: string }> = ({ title }) => {title}; + +- const [present, dismiss] = useIonModal(Modal, { subtitle: 'Wrong' }); ++ const [present, dismiss] = useIonModal(Modal, { title: 'Hello' }); +``` + +Props are read from the component rather than from `componentProps`, so a component declared inline needs its props annotated: + +```diff +- const [present, dismiss] = useIonModal(({ name }) =>
Hello {name}.
, { name: 'Dave' }); ++ const [present, dismiss] = useIonModal(({ name }: { name: string }) =>
Hello {name}.
, { name: 'Dave' }); +``` + +Passing a JSX element rather than a component is unchanged, and `componentProps` is not type checked in that case. + +`npx @ionic/migrate` reports the calls this affects and names what is wrong with each, but does not rewrite them, since the right fix depends on what the call was meant to do. For the inline case above, `--experimental` can annotate the parameter from the `componentProps` object literal being passed. + React Router v6 introduces several API changes that will require updates to your application's routing configuration: **Route Definition Changes** diff --git a/packages/migrate/README.md b/packages/migrate/README.md index d13a742b3ff..9bee3c15fa9 100644 --- a/packages/migrate/README.md +++ b/packages/migrate/README.md @@ -80,6 +80,9 @@ under [`docs/`](./docs). - Only `.ts` and `.tsx` are loaded into `ts-morph`, so `.js`/`.jsx` files and Angular inline templates (a `template:` string in a decorator) get the text-scan migrations but not the AST-based ones. +- No `tsconfig.json` is read. The type checker gets a fixed configuration, so a + `paths` alias doesn't resolve, and a migration reading types treats what it + can't reach as unknown rather than as nothing to report. - The template scanner is best-effort, not a full HTML parser. - Stylesheet scanning covers `.css` and `.scss` files. Styles inlined in a component decorator's `styles` array aren't read. diff --git a/packages/migrate/docs/v9.md b/packages/migrate/docs/v9.md index af4a25f6134..bbf24053981 100644 --- a/packages/migrate/docs/v9.md +++ b/packages/migrate/docs/v9.md @@ -38,8 +38,11 @@ the whole list for a vanilla app. | Change | Mode | | --- | --- | | `@ionic/react` + React 18 + React Router v6 bumps, drop `@types/react-router*` | auto | +| TypeScript raised to the 5.4 floor | auto | | `` removal, `component={X}` -> `element={}` | auto | | React Router v6: removed imports, `IonRedirect`, `render`/non-identifier `component`, route children, `history` prop, regex paths | report | +| `useIonModal`/`useIonPopover` calls whose `componentProps` no longer type check | report | +| An inline overlay component's unannotated props parameter | experimental | ### Vue @@ -81,6 +84,12 @@ The tool can't point at the code these changes affect, so check them against the in an Ionic lifecycle hook stops re-rendering. The report flags Angular 22 in `package.json`, but it doesn't find the affected components - `ng update` has a migration for that. +- Overlay hook calls whose component the type checker can't resolve. The tool + runs without your `tsconfig.json`, so a component behind a `paths` alias + (`@/components/Modal`), or one typed as `React.FC` with `@types/react` + uninstalled, is skipped rather than guessed at, and a build error can still be + waiting in one. A mismatch that only exists under `strict` is skipped too, so + that everything it does report is a real error. ## Notes on individual migrations @@ -88,6 +97,13 @@ The tool can't point at the code these changes affect, so check them against the shape. NgModule apps are flagged for manual migration instead. Neither fires unless the app loads Zone.js, since there is nothing to preserve otherwise and the provider fails to bootstrap without it. +- The overlay hook prop changes are report-only: a prop the component doesn't + declare is either a typo or a prop it should have declared, and a missing + required prop has no value to supply. The exception is a component written + inline at the call, whose props parameter `--experimental` annotates from the + `componentProps` object literal it is passed. That is a guess at intent + (`{ name: 'Dave' }` gives `{ name: string }`), and it declines anything but a + literal, or props that don't cover every name the component reads. - The component DOM/shadow-part changes are report-only. The right replacement depends on what the CSS rule was doing, and `ion-select`'s `part="inner"` has none at all. diff --git a/packages/migrate/src/ast/react-overlay-hooks.ts b/packages/migrate/src/ast/react-overlay-hooks.ts new file mode 100644 index 00000000000..705b95e80f4 --- /dev/null +++ b/packages/migrate/src/ast/react-overlay-hooks.ts @@ -0,0 +1,406 @@ +import { Node, SyntaxKind } from 'ts-morph'; +import type { CallExpression, ParameterDeclaration, SourceFile, Type, ts } from 'ts-morph'; + +/** The `@ionic/react` hooks that type `componentProps` against their component. */ +const HOOK_NAMES = new Set(['useIonModal', 'useIonPopover']); +const IONIC_REACT = '@ionic/react'; + +export interface OverlayHookCall { + call: CallExpression; + component: Node; + /** The component's name, when the argument is one a finding can name. */ + componentName?: string; + /** The `componentProps` argument, absent when the call omits it. */ + componentProps?: Node; + /** 1-based line of the call. */ + line: number; +} + +export interface DeclaredProp { + name: string; + required: boolean; +} + +/** + * What a component declares it accepts. A component with no props parameter has + * an empty {@link props} list, which is different from a type out of reach + * (`undefined`) and is reported differently. + */ +export interface PropsShape { + props: DeclaredProp[]; + /** The props type, when the component declares a parameter for it. */ + type?: Type; + /** + * Whether the type accepts keys it does not name, via an index signature. An + * open type can't produce an unknown-prop finding. + */ + open: boolean; + /** + * Whether the component is generic, so its prop types still hold unbound type + * parameters. Which props exist is still worth reading, but their types are not + * comparable until the hook instantiates them. + */ + generic: boolean; +} + +/** + * Expressions the file calls the overlay hooks by, covering both the named import + * (`useIonModal`, or whatever it was renamed to) and a namespace import's + * qualified form (`Ionic.useIonModal`). + * + * Read from the import rather than matched on the callee text, so an app's own + * function of the same name is not mistaken for the hook. + */ +function importedHookNames(file: SourceFile): Set { + const names = new Set(); + for (const imp of file.getImportDeclarations()) { + if (imp.getModuleSpecifierValue() !== IONIC_REACT) continue; + for (const named of imp.getNamedImports()) { + if (HOOK_NAMES.has(named.getName())) names.add(named.getAliasNode()?.getText() ?? named.getName()); + } + const namespace = imp.getNamespaceImport(); + if (namespace) { + for (const hook of HOOK_NAMES) names.add(`${namespace.getText()}.${hook}`); + } + } + return names; +} + +/** Every overlay hook call in `file`, in source order. */ +export function overlayHookCalls(file: SourceFile): OverlayHookCall[] { + const hooks = importedHookNames(file); + if (hooks.size === 0) return []; + + const calls: OverlayHookCall[] = []; + for (const call of file.getDescendantsOfKind(SyntaxKind.CallExpression)) { + if (!hooks.has(call.getExpression().getText())) continue; + const [component, componentProps] = call.getArguments(); + if (component === undefined) continue; + const named = Node.isIdentifier(component) || Node.isPropertyAccessExpression(component); + calls.push({ + call, + component, + ...(named ? { componentName: component.getText() } : {}), + ...(componentProps ? { componentProps } : {}), + line: call.getStartLineNumber(), + }); + } + return calls; +} + +function isElementArgument(component: Node): boolean { + return Node.isJsxElement(component) || Node.isJsxSelfClosingElement(component) || Node.isJsxFragment(component); +} + +/** An inline component reading props off a parameter that resolves to `{}`. */ +export interface UnannotatedInlineProps { + parameter: ParameterDeclaration; + /** Prop names the component body reads, in the order they are read. */ + reads: string[]; +} + +/** + * A component written inline at the call whose props parameter carries no type + * annotation, along with what it reads off that parameter. + * + * On its own `({ name }) => ...` infers `{ name: any }`, but at the hook it + * resolves to `{}`: `Props` is inferred from the component while `NoInfer` holds + * `componentProps` out of inference, and an unannotated parameter contributes + * nothing to infer from. The error is therefore in the component body, whatever + * `componentProps` holds. A parameter nothing is read from compiles, so an empty + * {@link reads} means there is nothing to report. + */ +export function unannotatedInlineProps(component: Node): UnannotatedInlineProps | undefined { + if (!Node.isArrowFunction(component) && !Node.isFunctionExpression(component)) return undefined; + const [parameter] = component.getParameters(); + if (parameter === undefined || parameter.getTypeNode() !== undefined) return undefined; + return { parameter, reads: propsRead(parameter, component) }; +} + +/** A property name, with the quotes off a string-literal key. */ +function propertyName(node: Node): string { + return Node.isStringLiteral(node) || Node.isNumericLiteral(node) ? String(node.getLiteralValue()) : node.getText(); +} + +function propsRead(parameter: ParameterDeclaration, component: Node): string[] { + const name = parameter.getNameNode(); + + // Destructured in the parameter list. A rest element names nothing. + if (Node.isObjectBindingPattern(name)) { + return name + .getElements() + .filter((element) => element.getDotDotDotToken() === undefined) + .map((element) => propertyName(element.getPropertyNameNode() ?? element.getNameNode())); + } + if (!Node.isIdentifier(name)) return []; + + // Read through the parameter's own name, either as `props.x` or by + // destructuring it in the body. Matched on the symbol rather than the text, so + // a nested function that shadows the name is not mistaken for a read of it. + // Without a symbol there is nothing to match references against, and comparing + // two undefined symbols would match every identifier in the body. + const parameterSymbol = name.getSymbol(); + if (parameterSymbol === undefined) return []; + + const reads: string[] = []; + for (const reference of component.getDescendantsOfKind(SyntaxKind.Identifier)) { + if (reference === name || reference.getSymbol() !== parameterSymbol) continue; + const parent = reference.getParent(); + if (Node.isPropertyAccessExpression(parent) && parent.getExpression() === reference) { + reads.push(parent.getName()); + } else if (Node.isVariableDeclaration(parent) && parent.getInitializer() === reference) { + const bound = parent.getNameNode(); + if (Node.isObjectBindingPattern(bound)) { + for (const element of bound.getElements()) { + if (element.getDotDotDotToken() === undefined) { + reads.push(propertyName(element.getPropertyNameNode() ?? element.getNameNode())); + } + } + } + } + } + return reads; +} + +/** + * What the hook signature checks `componentProps` against, or `undefined` when + * the call is outside what it checks or the answer is out of reach. + * + * Returning `undefined` means say nothing, which covers a JSX element (the + * permissive overload), a props type of `any` (permissive too), and a component + * type that never resolved, usually a `paths` alias or a package that isn't + * installed. + */ +export function declaredProps(hookCall: OverlayHookCall): PropsShape | undefined { + const { call, component } = hookCall; + if (isElementArgument(component)) return undefined; + + // An explicit type argument pins `Props`, so the component is not consulted. + const [typeArg] = call.getTypeArguments(); + if (typeArg) return shapeOf(typeArg.getType()); + + const componentType = component.getType(); + const [callSignature] = componentType.getCallSignatures(); + if (callSignature) { + const generic = callSignature.getTypeParameters().length > 0; + const [propsParam] = callSignature.getParameters(); + // No parameter means the component accepts no props at all. + if (propsParam === undefined) return { props: [], open: false, generic }; + const declaration = propsParam.getDeclarations()[0]; + return declaration ? shapeOf(propsParam.getTypeAtLocation(declaration), generic) : undefined; + } + + const [constructSignature] = componentType.getConstructSignatures(); + if (constructSignature) { + const props = constructSignature.getReturnType().getProperty('props'); + const declaration = props?.getDeclarations()[0]; + return declaration + ? shapeOf(props!.getTypeAtLocation(declaration), constructSignature.getTypeParameters().length > 0) + : undefined; + } + return undefined; +} + +/** Read a resolved props type into a {@link PropsShape}. */ +function shapeOf(type: Type, generic = false): PropsShape | undefined { + // Like `any`, these mean the type resolved to nothing usable. + if (type.isAny() || type.isUnknown() || type.isNever()) return undefined; + return { + type, + props: type.getProperties().map((prop) => ({ + name: prop.getName(), + required: !prop.isOptional(), + })), + open: type.getStringIndexType() !== undefined || type.getNumberIndexType() !== undefined, + generic, + }; +} + +/** + * Property names of the type passed as `componentProps`, or `undefined` when the + * type says nothing about them. + * + * The case that matters is `any`: it satisfies the signature whatever the + * component declares, so the call still compiles, and reading no properties off + * it would look like a call passing nothing at all. + */ +export function passedPropNames(componentProps: Node): string[] | undefined { + const type = componentProps.getType(); + if (type.isAny() || type.isUnknown() || type.isTypeParameter()) return undefined; + return type.getProperties().map((prop) => prop.getName()); +} + +/** + * Prop names written directly in a fresh object literal at the call, or + * `undefined` when `componentProps` isn't one. + * + * The only input an unknown-prop finding may be drawn from, since excess-property + * checking only applies to a fresh literal and to the members spelled out in it. + * A variable passed by name and a key arriving through a spread are both accepted + * however little they match. A key written alongside that spread is still checked. + */ +export function writtenPropNames(componentProps: Node): string[] | undefined { + if (!Node.isObjectLiteralExpression(componentProps)) return undefined; + const names: string[] = []; + for (const prop of componentProps.getProperties()) { + // A spread contributes keys from a type, not from this literal, and they are + // not excess-property checked. + if (Node.isSpreadAssignment(prop)) continue; + const nameNode = Node.isPropertyAssignment(prop) || + Node.isShorthandPropertyAssignment(prop) || + Node.isMethodDeclaration(prop) || + Node.isGetAccessorDeclaration(prop) || + Node.isSetAccessorDeclaration(prop) + ? prop.getNameNode() + : undefined; + if (nameNode === undefined) continue; + // A computed key isn't a known name to check against. + if (Node.isComputedPropertyName(nameNode)) continue; + names.push(propertyName(nameNode)); + } + return names; +} + +/** A prop passed with a type the component does not accept. */ +export interface PropMismatch { + name: string; + /** The type the component declares, as source text. */ + declared: string; + /** The type `componentProps` passes, as source text. */ + passed: string; +} + +/** + * Props present on both sides whose passed type the declared type does not + * accept. Empty when the comparison can't be made. + * + * The checker is asked directly rather than the type texts compared, since only + * it knows about widening, unions, and variance. It runs with the compiler + * defaults from `context.ts`, not the app's `tsconfig.json`, so a mismatch that + * only exists under `strict` compares as assignable and goes unreported. That + * direction is deliberate: anything reported here fails under any setting. + */ +export function propMismatches(declared: PropsShape, componentProps: Node): PropMismatch[] { + const declaredType = declared.type; + // A generic component's prop types still hold unbound type parameters, and + // nothing is assignable to a bare `T`. The hook instantiates them at the call, + // so comparing here reports calls that compile. + if (declaredType === undefined || declared.generic) return []; + + // The checker's `isTypeAssignableTo` is internal rather than part of ts-morph's + // surface, so a release that drops it degrades to reporting nothing here rather + // than throwing mid-run. + const checker = componentProps.getProject().getTypeChecker().compilerObject as ts.TypeChecker & { + isTypeAssignableTo?: (source: ts.Type, target: ts.Type) => boolean; + }; + if (typeof checker.isTypeAssignableTo !== 'function') return []; + + const passedType = componentProps.getType(); + const mismatches: PropMismatch[] = []; + for (const passedProp of passedType.getProperties()) { + const declaredProp = declaredType.getProperty(passedProp.getName()); + if (declaredProp === undefined) continue; + + const passedDeclaration = passedProp.getDeclarations()[0]; + const declaredDeclaration = declaredProp.getDeclarations()[0]; + if (passedDeclaration === undefined || declaredDeclaration === undefined) continue; + + const passed = passedProp.getTypeAtLocation(passedDeclaration); + const target = declaredProp.getTypeAtLocation(declaredDeclaration); + if (checker.isTypeAssignableTo(passed.compilerType, target.compilerType)) continue; + mismatches.push({ + name: passedProp.getName(), + declared: target.getText(declaredDeclaration), + passed: passed.getText(passedDeclaration), + }); + } + return mismatches; +} + +/** + * A prop name as it has to be written in a type literal, or `undefined` when it + * can't be written safely. Anything that isn't a plain identifier (`data-test`, + * `aria-label`) needs quoting to parse, and a name carrying a quote of its own + * declines instead. + */ +function memberName(name: string): string | undefined { + if (/^[A-Za-z_$][\w$]*$/.test(name)) return name; + return /['\\\r\n]/.test(name) ? undefined : `'${name}'`; +} + +export interface PropsAnnotation { + /** The type literal to write, e.g. `{ name: string }`. */ + text: string; + names: string[]; +} + +/** + * A props annotation written from the `componentProps` passed at a call, or + * `undefined` when one can't be printed. + * + * Only an object literal qualifies. A variable or a spread would need the type it + * resolves to, and printing that names types the file may not have imported, so + * those calls are left to be reported instead. A member whose own type prints as + * a module path (`import("/path").Foo`) or as `typeof` a binding the file may not + * have is unusable as source text for the same reason, and declines the whole + * annotation rather than half of one. + */ +export function annotationFor(componentProps: Node): PropsAnnotation | undefined { + if (!Node.isObjectLiteralExpression(componentProps)) return undefined; + // Only plain `name: value` and shorthand `name` members carry a name and a type + // this can read. A spread contributes keys from somewhere else entirely. + const named = componentProps.getProperties().every((prop) => { + if (!Node.isPropertyAssignment(prop) && !Node.isShorthandPropertyAssignment(prop)) return false; + return !Node.isComputedPropertyName(prop.getNameNode()); + }); + if (!named) return undefined; + + const members: string[] = []; + const names: string[] = []; + for (const prop of componentProps.getType().getProperties()) { + const declaration = prop.getDeclarations()[0]; + if (declaration === undefined) return undefined; + const name = memberName(prop.getName()); + if (name === undefined) return undefined; + const text = prop.getTypeAtLocation(declaration).getText(declaration); + if (/\bimport\(|\btypeof\b/.test(text)) return undefined; + members.push(`${name}${prop.isOptional() ? '?' : ''}: ${text}`); + names.push(prop.getName()); + } + return members.length > 0 ? { text: `{ ${members.join('; ')} }`, names } : undefined; +} + +/** + * The name in `candidates` closest to `name`, when one is close enough to be + * worth suggesting. Only a single best match within two edits qualifies, so a + * prop that isn't declared reads as undeclared rather than as a typo. + */ +export function closestName(name: string, candidates: string[]): string | undefined { + let best: string | undefined; + let bestDistance = Infinity; + let tied = false; + for (const candidate of candidates) { + const distance = editDistance(name.toLowerCase(), candidate.toLowerCase()); + if (distance < bestDistance) { + best = candidate; + bestDistance = distance; + tied = false; + } else if (distance === bestDistance) { + tied = true; + } + } + return best !== undefined && bestDistance <= 2 && !tied ? best : undefined; +} + +/** Levenshtein distance, for {@link closestName}. */ +function editDistance(a: string, b: string): number { + let previous = Array.from({ length: b.length + 1 }, (_, i) => i); + for (let i = 1; i <= a.length; i++) { + const current = [i]; + for (let j = 1; j <= b.length; j++) { + current[j] = Math.min(previous[j] + 1, current[j - 1] + 1, previous[j - 1] + (a[i - 1] === b[j - 1] ? 0 : 1)); + } + previous = current; + } + return previous[b.length]; +} diff --git a/packages/migrate/src/context.ts b/packages/migrate/src/context.ts index 41e1555716b..ba62b5f596c 100644 --- a/packages/migrate/src/context.ts +++ b/packages/migrate/src/context.ts @@ -5,6 +5,22 @@ import { Project, QuoteKind } from 'ts-morph'; /** ts-morph should emit single-quoted strings to match Ionic/Angular style. */ const MANIPULATION_SETTINGS = { quoteKind: QuoteKind.Single } as const; +/** + * Compiler options every context shares. No `tsconfig.json` is read (see + * {@link createDiskContext}), so this is the whole configuration the type checker + * gets: a project's `paths` aliases stay unresolvable, and a migration reading + * types has to treat that as "unknown", never as "no problem here". + * + * Setting `allowJs: false` scopes the AST migrations to `.ts`/`.tsx`. + * + * Adding `esModuleInterop` is what lets a type-aware migration resolve anything: + * React publishes its types as `export = React`, so without it every + * `React.FC` degrades to `any` and prop analysis silently finds nothing. + * Nothing here emits or reads diagnostics, so the flag only widens type + * resolution. + */ +const COMPILER_OPTIONS = { allowJs: false, esModuleInterop: true } as const; + /** Join a root dir and a relative path using posix separators. */ function join(root: string, rel: string): string { return `${root.replace(/\/$/, '')}/${rel.replace(/^\//, '')}`; @@ -167,6 +183,7 @@ export function createInMemoryContext( ): MigrationContext { const project = new Project({ useInMemoryFileSystem: true, + compilerOptions: COMPILER_OPTIONS, manipulationSettings: MANIPULATION_SETTINGS, }); const fs = project.getFileSystem(); @@ -199,7 +216,7 @@ export function createInMemoryContext( export function createDiskContext(rootDir: string): MigrationContext { const project = new Project({ skipAddingFilesFromTsConfig: true, - compilerOptions: { allowJs: false }, + compilerOptions: COMPILER_OPTIONS, manipulationSettings: MANIPULATION_SETTINGS, }); // Load the whole tree (minus build/vendor dirs), not just `src/`, so AST diff --git a/packages/migrate/src/migrations/index.ts b/packages/migrate/src/migrations/index.ts index 65ae39e5b78..4d8a979a9d8 100644 --- a/packages/migrate/src/migrations/index.ts +++ b/packages/migrate/src/migrations/index.ts @@ -12,6 +12,9 @@ import { angularVersion } from './v9/angular-version.js'; import { angularBrowserPolicy } from './v9/angular-browser-policy.js'; import { angularBrowserPolicyManual } from './v9/angular-browser-policy-manual.js'; import { reactDeps } from './v9/react-deps.js'; +import { reactTypescript } from './v9/react-typescript.js'; +import { reactOverlayHookProps } from './v9/react-overlay-hook-props.js'; +import { reactOverlayHookPropsManual } from './v9/react-overlay-hook-props-manual.js'; import { reactRouter6Routes } from './v9/react-router-6-routes.js'; import { reactRouter6Code } from './v9/react-router-6-code.js'; import { vueDeps } from './v9/vue-deps.js'; @@ -51,6 +54,9 @@ export const allMigrations: Migration[] = [ angularBrowserPolicy, angularBrowserPolicyManual, reactDeps, + reactTypescript, + reactOverlayHookProps, + reactOverlayHookPropsManual, reactRouter6Routes, reactRouter6Code, vueDeps, diff --git a/packages/migrate/src/migrations/v9/react-deps.ts b/packages/migrate/src/migrations/v9/react-deps.ts index b6cc226017d..a1a03d30a9a 100644 --- a/packages/migrate/src/migrations/v9/react-deps.ts +++ b/packages/migrate/src/migrations/v9/react-deps.ts @@ -7,7 +7,7 @@ import { createDepsMigration } from '../../ast/deps-migration.js'; * `react-router-6-code`. * * React is raised only to the 18 floor v9 requires; a newer major is the app's - * call. + * call. TypeScript is handled by `react-typescript`. * * Refer to https://ionicframework.com/docs/updating/9-0#react */ diff --git a/packages/migrate/src/migrations/v9/react-overlay-hook-props-manual.ts b/packages/migrate/src/migrations/v9/react-overlay-hook-props-manual.ts new file mode 100644 index 00000000000..6949d207e7e --- /dev/null +++ b/packages/migrate/src/migrations/v9/react-overlay-hook-props-manual.ts @@ -0,0 +1,142 @@ +import { + closestName, + declaredProps, + writtenPropNames, + overlayHookCalls, + passedPropNames, + propMismatches, + unannotatedInlineProps, +} from '../../ast/react-overlay-hooks.js'; +import type { OverlayHookCall } from '../../ast/react-overlay-hooks.js'; +import type { Finding, Migration } from '../../types.js'; + +/** + * Quote and join prop names for a sentence. No article, since `a`/`an` picked + * from the first letter gets names like `url` and `user` wrong. + */ +function propList(names: string[]): string { + const quoted = names.map((name) => `"${name}"`); + if (quoted.length === 1) return quoted[0]; + const last = quoted.pop(); + return `${quoted.join(', ')} and ${last}`; +} + +/** + * What stops one call from compiling, as report details. Empty when the call is + * fine or when the answer is out of reach. + * + * Derived here rather than in `detect` so the checks read in the order they + * interact: a suggestion for a misspelled prop suppresses the missing-prop + * finding it would otherwise duplicate. + */ +function callProblems(hookCall: OverlayHookCall): string[] { + const { component, componentProps } = hookCall; + // A call expression or a conditional has no name to quote, so the wording falls + // back to naming it by role. + const componentName = hookCall.componentName ?? 'The component'; + + // Checked before the component's type, which is misleading for this shape: the + // break is in the component body (see `unannotatedInlineProps`). Once + // `--experimental` has annotated the parameter, a later run says nothing here. + const inline = unannotatedInlineProps(component); + if (inline !== undefined) { + return inline.reads.length > 0 + ? [ + `The inline component reads ${propList([...new Set(inline.reads)])} off an unannotated ` + + `props parameter, which resolves to {}`, + ] + : []; + } + + const declared = declaredProps(hookCall); + if (declared === undefined) return []; + const declaredNames = declared.props.map((prop) => prop.name); + const required = declared.props.filter((prop) => prop.required).map((prop) => prop.name); + + // The signature makes `componentProps` an argument the caller can't drop once + // the component declares a prop it can't do without. + if (componentProps === undefined) { + return required.length > 0 + ? [`${componentName} requires ${propList(required)}, so componentProps can no longer be omitted`] + : []; + } + + // Nothing readable was passed (`props as any`), and the signature accepts it. + const passed = passedPropNames(componentProps); + if (passed === undefined) return []; + + const problems: string[] = []; + // A misspelled prop also leaves the prop it meant to pass missing. Reporting + // both halves describes one mistake twice, so a name a suggestion accounts for + // is held back from the missing-prop check below. + const suggested = new Set(); + // Excess-property checking only fires on keys written in a fresh object literal + // (see `writtenPropNames`), against a target that names at least one property. + // A component declaring none accepts anything, and an open type names keys it + // doesn't list. + const written = writtenPropNames(componentProps); + if (written !== undefined && declaredNames.length > 0 && !declared.open) { + for (const name of written) { + if (declaredNames.includes(name)) continue; + const suggestion = closestName(name, declaredNames); + if (suggestion) suggested.add(suggestion); + problems.push( + `${componentName} has no "${name}" prop` + (suggestion ? `. Did you mean "${suggestion}"?` : '') + ); + } + } + + // One finding for the whole set. A call missing three props is one thing to + // fix, and three lines pointing at the same line read as three. + const missing = required.filter((name) => !passed.includes(name) && !suggested.has(name)); + if (missing.length > 0) { + problems.push(`${componentName} requires ${propList(missing)}, which componentProps does not pass`); + } + + for (const mismatch of propMismatches(declared, componentProps)) { + problems.push( + `${componentName}'s "${mismatch.name}" prop is ${mismatch.declared}, ` + + `and componentProps passes ${mismatch.passed}` + ); + } + return problems; +} + +/** + * Reports `useIonModal`/`useIonPopover` calls that stop compiling now that the + * hooks type `componentProps` against the component they are given instead of + * accepting `any`. + * + * Report-only, because every case turns on what the developer meant. A prop the + * component doesn't declare is either a typo or a prop the component should have + * declared, a missing required prop has no value to supply for it, and nothing + * here can tell those apart. + * + * It stays quiet, deliberately, about a component whose type doesn't resolve (a + * `paths` alias, a package without types installed), a props type of `any`, the + * JSX element form, and a mismatch that only exists under `strict` (see + * `propMismatches`). A false finding sends someone editing code that compiles. + * + * Refer to https://ionicframework.com/docs/updating/9-0#typed-overlay-hook-props + */ +export const reactOverlayHookPropsManual: Migration = { + id: 'react-overlay-hook-props-manual', + framework: 'react', + fromMajor: 8, + toMajor: 9, + status: 'stable', + docsUrl: 'https://ionicframework.com/docs/updating/9-0#typed-overlay-hook-props', + + detect(ctx) { + const findings: Finding[] = []; + for (const file of ctx.project.getSourceFiles()) { + const filePath = ctx.relative(file.getFilePath()); + for (const hookCall of overlayHookCalls(file)) { + for (const detail of callProblems(hookCall)) { + findings.push({ filePath, line: hookCall.line, detail }); + } + } + } + return findings; + }, +}; diff --git a/packages/migrate/src/migrations/v9/react-overlay-hook-props.ts b/packages/migrate/src/migrations/v9/react-overlay-hook-props.ts new file mode 100644 index 00000000000..8ead76d2f76 --- /dev/null +++ b/packages/migrate/src/migrations/v9/react-overlay-hook-props.ts @@ -0,0 +1,80 @@ +import type { ParameterDeclaration } from 'ts-morph'; + +import { annotationFor, overlayHookCalls, unannotatedInlineProps } from '../../ast/react-overlay-hooks.js'; +import type { MigrationContext } from '../../context.js'; +import type { Migration } from '../../types.js'; + +interface Annotation { + filePath: string; + line: number; + parameter: ParameterDeclaration; + text: string; +} + +/** + * Every inline component this migration can annotate. Shared by detect and fix so + * the report and the edit can't disagree about which calls are covered. + */ +function annotations(ctx: MigrationContext): Annotation[] { + const found: Annotation[] = []; + for (const file of ctx.project.getSourceFiles()) { + const filePath = ctx.relative(file.getFilePath()); + for (const { call, component, componentProps, line } of overlayHookCalls(file)) { + if (componentProps === undefined) continue; + // An explicit type argument pins `Props`, so an annotation derived from + // `componentProps` could contradict what the hook actually checks against. + if (call.getTypeArguments().length > 0) continue; + const inline = unannotatedInlineProps(component); + // Nothing is read off the parameter, so there is no error to annotate away. + if (inline === undefined || inline.reads.length === 0) continue; + const annotation = annotationFor(componentProps); + if (annotation === undefined) continue; + // Annotating from props that don't cover every name the component reads + // would trade one compile error for another, and silence the report that + // would have described it. + if (inline.reads.some((name) => !annotation.names.includes(name))) continue; + found.push({ filePath, line, parameter: inline.parameter, text: annotation.text }); + } + } + return found; +} + +/** + * Annotates the props parameter of a component written inline at a + * `useIonModal`/`useIonPopover` call, from the `componentProps` it is passed. + * + * Experimental because the annotation is derived from values rather than read + * from a declaration: `{ name: 'Dave' }` gives `{ name: string }`, which is often + * but not always what was wanted. Anything it can't be printed from is left to + * `react-overlay-hook-props-manual` to report. + * + * Runs before that migration, which reports the same calls: migrations are + * selected in id order, so the report re-reads an annotated parameter and stays + * quiet. Renaming either id breaks that. + * + * Refer to https://ionicframework.com/docs/updating/9-0#typed-overlay-hook-props + */ +export const reactOverlayHookProps: Migration = { + id: 'react-overlay-hook-props', + framework: 'react', + fromMajor: 8, + toMajor: 9, + status: 'experimental', + docsUrl: 'https://ionicframework.com/docs/updating/9-0#typed-overlay-hook-props', + + detect(ctx) { + return annotations(ctx).map(({ filePath, line, text }) => ({ + filePath, + line, + detail: `annotate the inline component's props parameter as ${text}`, + })); + }, + + fix(ctx) { + // Bottom-up, so annotating one call can't move the positions of the calls + // still to be visited. + for (const { parameter, text } of annotations(ctx).reverse()) { + parameter.setType(text); + } + }, +}; diff --git a/packages/migrate/src/migrations/v9/react-typescript.ts b/packages/migrate/src/migrations/v9/react-typescript.ts new file mode 100644 index 00000000000..3fef0c329fd --- /dev/null +++ b/packages/migrate/src/migrations/v9/react-typescript.ts @@ -0,0 +1,17 @@ +import { createDepsMigration } from '../../ast/deps-migration.js'; + +/** + * Ionic 9 requires TypeScript 5.4 or later. `@ionic/react`'s published types use + * `NoInfer`, which TypeScript added in 5.4. A higher pin is left alone. + * + * Pinned with a caret, unlike the tilde `angular-typescript` uses: React has no + * narrow peer range to satisfy, so any later 5.x is fine. + * + * Refer to https://ionicframework.com/docs/updating/9-0#react-typescript + */ +export const reactTypescript = createDepsMigration({ + id: 'react-typescript', + framework: 'react', + docsUrl: 'https://ionicframework.com/docs/updating/9-0#react-typescript', + bumps: [['typescript', '^5.4.0']], +}); diff --git a/packages/migrate/test/helpers/react.ts b/packages/migrate/test/helpers/react.ts new file mode 100644 index 00000000000..aa10da485bc --- /dev/null +++ b/packages/migrate/test/helpers/react.ts @@ -0,0 +1,61 @@ +import { createInMemoryContext } from '../../src/context.js'; +import type { MigrationContext } from '../../src/context.js'; + +/** + * A minimal stand-in for `@types/react`, holding the component shapes the overlay + * hook analysis reads: the `FunctionComponent` call signature, `Component`'s + * `props` member, and `memo`'s identity return. Written into the in-memory + * project so a fixture's `React.FC` resolves to a real props type instead + * of `any` - the same thing an installed `@types/react` gives the disk context. + * + * Declaring it as `export =` is what makes `esModuleInterop` load-bearing here, + * as it is in the published types. + */ +export const REACT_TYPES = ` +declare namespace React { + type ReactNode = any; + interface FunctionComponent

{ + (props: P): ReactNode; + } + type FC

= FunctionComponent

; + class Component

{ + props: Readonly

; + state: Readonly; + render(): ReactNode; + } + interface NamedExoticComponent

{ + (props: P): ReactNode; + displayName?: string; + } + function memo

(component: FunctionComponent

): NamedExoticComponent

; +} +export = React; +export as namespace React; +`; + +/** A stand-in for `@ionic/react`, so a fixture's hook import resolves. */ +const IONIC_REACT_TYPES = ` +export declare function useIonModal(component: any, componentProps?: any): [(opts?: any) => void, (data?: any, role?: string) => void]; +export declare function useIonPopover(component: any, componentProps?: any): [(opts?: any) => void, (data?: any, role?: string) => void]; +`; + +/** + * An in-memory React project with `react` and `@ionic/react` types installed. + * Entries in `files` are written as given, so a test can lay out components + * across files the way an app does. + */ +export function reactProject(files: Record): MigrationContext { + return createInMemoryContext({ + 'package.json': JSON.stringify({ dependencies: { '@ionic/react': '^8.0.0', react: '^18.0.0' } }, null, 2), + 'node_modules/@types/react/package.json': JSON.stringify({ name: '@types/react', types: 'index.d.ts' }), + 'node_modules/@types/react/index.d.ts': REACT_TYPES, + 'node_modules/@ionic/react/package.json': JSON.stringify({ name: '@ionic/react', types: 'index.d.ts' }), + 'node_modules/@ionic/react/index.d.ts': IONIC_REACT_TYPES, + ...files, + }); +} + +/** The current text of a file in a {@link reactProject}, after a fix has run. */ +export function readSource(ctx: MigrationContext, relPath: string): string { + return ctx.project.getSourceFileOrThrow(`${ctx.rootDir}/${relPath}`).getFullText(); +} diff --git a/packages/migrate/test/react-overlay-hook-props-manual.test.ts b/packages/migrate/test/react-overlay-hook-props-manual.test.ts new file mode 100644 index 00000000000..3146f2f354c --- /dev/null +++ b/packages/migrate/test/react-overlay-hook-props-manual.test.ts @@ -0,0 +1,468 @@ +import { describe, expect, it } from 'vitest'; + +import { reactOverlayHookPropsManual as migration } from '../src/migrations/v9/react-overlay-hook-props-manual.js'; +import { reactProject } from './helpers/react.js'; + +const BODY = ` +import React from 'react'; +export const Body: React.FC<{ title: string; count?: number }> = ({ title }) => null as any; +`; + +/** A page whose single hook call is `body`, with `Body` in scope. */ +function page(body: string): Record { + return { + 'src/Body.tsx': BODY, + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; +import { Body } from './Body'; + +export const Page = () => { + ${body} + return null as any; +}; +`, + }; +} + +describe('react-overlay-hook-props-manual', () => { + it('reports a prop the component does not declare', () => { + const ctx = reactProject({ + 'src/Body.tsx': BODY, + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; +import { Body } from './Body'; + +export const Page = () => { + const [present] = useIonModal(Body, { titel: 'Hello' }); + return null as any; +}; +`, + }); + + const findings = migration.detect(ctx); + + expect(findings).toEqual([ + { + filePath: 'src/Page.tsx', + line: 6, + detail: 'Body has no "titel" prop. Did you mean "title"?', + }, + ]); + }); + + it('reports a required prop componentProps leaves out', () => { + const ctx = reactProject(page(`const [present] = useIonModal(Body, { count: 1 });`)); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Body requires "title", which componentProps does not pass', + ]); + }); + + it('collects every missing required prop into one finding per call', () => { + const ctx = reactProject({ + 'src/Page.tsx': ` +import React from 'react'; +import { useIonModal } from '@ionic/react'; + +const Body: React.FC<{ type: string; count: number; onIncrement: () => void }> = () => null as any; + +export const Page = () => { + const [present] = useIonModal(Body, {}); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Body requires "type", "count" and "onIncrement", which componentProps does not pass', + ]); + }); + + it('reports a prop whose value does not match the declared type', () => { + const ctx = reactProject(page(`const [present] = useIonModal(Body, { title: 1 });`)); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Body\'s "title" prop is string, and componentProps passes number', + ]); + }); + + it('says nothing about props passed to a component that declares none', () => { + // Verified against tsc: a props-less component's Props resolves to `{}`, and + // TypeScript skips excess-property checking against an empty target type, so + // this compiles. It looks wrong and is not. + const ctx = reactProject({ + 'src/Page.tsx': ` +import { useIonPopover } from '@ionic/react'; + +const Popover = () => null as any; + +export const Page = () => { + const [present, dismiss] = useIonPopover(Popover, { onDismiss: () => dismiss() }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('says nothing about an extra key on a componentProps variable', () => { + // Excess-property checking only applies to a fresh object literal. A variable + // is assignable as long as it satisfies the declared props. + const ctx = reactProject({ + 'src/Page.tsx': ` +import React from 'react'; +import { useIonModal } from '@ionic/react'; + +const Body: React.FC<{ title?: string }> = () => null as any; + +export const Page = () => { + const props = { title: 'Hello', nope: 1 }; + const [present] = useIonModal(Body, props); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('says nothing about an extra key that arrives through a spread', () => { + const ctx = reactProject({ + 'src/Page.tsx': ` +import React from 'react'; +import { useIonModal } from '@ionic/react'; + +const Body: React.FC<{ title?: string }> = () => null as any; + +export const Page = () => { + const extra = { title: 'Hello', nope: 1 }; + const [present] = useIonModal(Body, { ...extra }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('reports a key written directly alongside a spread', () => { + const ctx = reactProject({ + 'src/Page.tsx': ` +import React from 'react'; +import { useIonModal } from '@ionic/react'; + +const Body: React.FC<{ title?: string }> = () => null as any; + +export const Page = () => { + const extra = { title: 'Hello' }; + const [present] = useIonModal(Body, { nope: 1, ...extra }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual(['Body has no "nope" prop']); + }); + + it('reports a call that omits componentProps for a component that requires them', () => { + const ctx = reactProject(page(`const [present] = useIonModal(Body);`)); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Body requires "title", so componentProps can no longer be omitted', + ]); + }); + + it('says nothing about a call that already type checks', () => { + const ctx = reactProject(page(`const [present] = useIonModal(Body, { title: 'Hello', count: 1 });`)); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('reports an inline component that reads a prop off an unannotated parameter', () => { + // Props are read from the component, so an unannotated parameter resolves to + // `{}` and reading a name off it is the error. + const ctx = reactProject( + page(`const [present] = useIonModal(({ name }) => name as any, { name: 'Dave' });`) + ); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'The inline component reads "name" off an unannotated props parameter, which resolves to {}', + ]); + }); + + it('reports an inline component reading props even when componentProps is empty', () => { + // The error is in the component body, so it does not depend on what is + // passed. Verified against tsc: this is TS2339 either way. + const ctx = reactProject(page(`const [present] = useIonModal(({ name }) => name as any, {});`)); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'The inline component reads "name" off an unannotated props parameter, which resolves to {}', + ]); + }); + + it('reports an inline component reading a prop through its parameter name', () => { + const ctx = reactProject( + page(`const [present] = useIonModal((props) => props.name as any, { name: 'Dave' });`) + ); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'The inline component reads "name" off an unannotated props parameter, which resolves to {}', + ]); + }); + + it('says nothing about an inline component that never reads its parameter', () => { + // Nothing is read off `{}`, so this compiles however much is passed. + const ctx = reactProject(page(`const [present] = useIonModal((props) => 'literal' as any, { anything: 1 });`)); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('does not count a shadowed parameter in a nested function as a read', () => { + const ctx = reactProject({ + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; + +export const Page = () => { + const [present] = useIonModal((props) => { + const inner = (props: { other: string }) => props.other; + return inner({ other: 'x' }) as any; + }, { anything: 1 }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('says nothing about an inline component that only collects a rest object', () => { + const ctx = reactProject(page(`const [present] = useIonModal(({ ...rest }) => rest as any, { anything: 1 });`)); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('checks against an explicit type argument rather than the component', () => { + const ctx = reactProject({ + 'src/Body.tsx': BODY, + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; +import { Body } from './Body'; + +interface Pinned { + heading: string; +} + +export const Page = () => { + const [present] = useIonModal(Body, { title: 'Hello' }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Body has no "title" prop', + 'Body requires "heading", which componentProps does not pass', + ]); + }); + + it('follows a hook reached through a namespace import', () => { + const ctx = reactProject({ + 'src/Body.tsx': BODY, + 'src/Page.tsx': ` +import * as Ionic from '@ionic/react'; +import { Body } from './Body'; + +export const Page = () => { + const [present] = Ionic.useIonModal(Body, { titel: 'Hello' }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Body has no "titel" prop. Did you mean "title"?', + ]); + }); + + it('follows a hook renamed on import', () => { + const ctx = reactProject({ + 'src/Body.tsx': BODY, + 'src/Page.tsx': ` +import { useIonModal as useModal } from '@ionic/react'; +import { Body } from './Body'; + +export const Page = () => { + const [present] = useModal(Body, { titel: 'Hello' }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Body has no "titel" prop. Did you mean "title"?', + ]); + }); + + it("ignores an app's own function that shares a hook name", () => { + const ctx = reactProject({ + 'src/Body.tsx': BODY, + 'src/Page.tsx': ` +import { Body } from './Body'; + +function useIonModal(component: any, componentProps?: any) { + return [component, componentProps]; +} + +export const Page = () => { + const [present] = useIonModal(Body, { titel: 'Hello' }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('says nothing about a generic component whose prop types are not yet bound', () => { + // The `value` prop is declared as `T`, which nothing is assignable to until + // the hook instantiates it. Comparing against it would report a call that + // compiles. + const ctx = reactProject({ + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; + +function Generic({ value }: { value: T }) { + return value as any; +} + +export const Page = () => { + const [present] = useIonModal(Generic, { value: 1 }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('still reports an unknown prop on a generic component', () => { + // Only the prop types are unbound. Which props exist is still known. + const ctx = reactProject({ + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; + +function Generic({ value }: { value: T }) { + return value as any; +} + +export const Page = () => { + const [present] = useIonModal(Generic, { valeu: 1 }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'Generic has no "valeu" prop. Did you mean "value"?', + ]); + }); + + it('says nothing when componentProps is passed as any', () => { + // Passing `any` satisfies the signature, so the call still compiles, and there + // is nothing to read the passed keys from either. + const ctx = reactProject(page(`const [present] = useIonModal(Body, {} as any);`)); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('reads through a component that is not a plain reference', () => { + // A call expression, a conditional, and spread arguments all reach the + // analysis. None of them may throw, whatever the type resolves to. + const ctx = reactProject({ + 'src/Page.tsx': ` +import React from 'react'; +import { useIonModal } from '@ionic/react'; + +const Body: React.FC<{ title: string }> = () => null as any; +const Other: React.FC<{ other: string }> = () => null as any; +declare const flag: boolean; +declare const args: unknown; +declare function getBody(): React.FC<{ title: string }>; + +export const Page = () => { + const [a] = useIonModal(getBody(), { titel: 'x' }); + const [b] = useIonModal(flag ? Body : Other, { title: 'x' }); + const [c] = useIonModal(...(args as [any, any])); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx).map((finding) => finding.detail)).toEqual([ + 'The component has no "titel" prop. Did you mean "title"?', + ]); + }); + + it('says nothing about the JSX element form, whose props are already bound', () => { + const ctx = reactProject( + page(`const [present] = useIonModal(, { anything: true });`) + ); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('says nothing about a component typed with any, which stays permissive', () => { + const ctx = reactProject({ + 'src/Page.tsx': ` +import React from 'react'; +import { useIonModal } from '@ionic/react'; + +const Body: React.FC = () => null as any; + +export const Page = () => { + const [present] = useIonModal(Body, { anything: true }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('says nothing when the component type is out of reach', () => { + // A component from a package with no types installed. Reporting a guess here + // would be worse than the silence. + const ctx = reactProject({ + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; +import { Body } from 'some-untyped-package'; + +export const Page = () => { + const [present] = useIonModal(Body, { title: 'Hello' }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('says nothing about a component whose props are all optional', () => { + const ctx = reactProject({ + 'src/Page.tsx': ` +import React from 'react'; +import { useIonModal } from '@ionic/react'; + +const Body: React.FC<{ count?: number }> = () => null as any; + +export const Page = () => { + const [present] = useIonModal(Body); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); +}); diff --git a/packages/migrate/test/react-overlay-hook-props.test.ts b/packages/migrate/test/react-overlay-hook-props.test.ts new file mode 100644 index 00000000000..6be5d17957c --- /dev/null +++ b/packages/migrate/test/react-overlay-hook-props.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, it } from 'vitest'; + +import { allMigrations } from '../src/migrations/index.js'; +import { reactOverlayHookProps as migration } from '../src/migrations/v9/react-overlay-hook-props.js'; +import { reactOverlayHookPropsManual } from '../src/migrations/v9/react-overlay-hook-props-manual.js'; +import { selectMigrations } from '../src/registry.js'; +import { readSource, reactProject } from './helpers/react.js'; + +/** A page whose single hook call is `body`. */ +function page(body: string): Record { + return { + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; + +export const Page = () => { + ${body} + return null as any; +}; +`, + }; +} + +describe('react-overlay-hook-props', () => { + it('annotates an inline component from the props it is passed', () => { + const ctx = reactProject(page(`const [present] = useIonModal(({ name }) => name as any, { name: 'Dave' });`)); + + migration.fix!(ctx); + + expect(readSource(ctx, 'src/Page.tsx')).toContain( + `useIonModal(({ name }: { name: string }) => name as any, { name: 'Dave' })` + ); + }); + + it('quotes a prop name that is not a valid identifier', () => { + const ctx = reactProject( + page(`const [present] = useIonModal(({ 'data-test': id }) => id as any, { 'data-test': 'x' });`) + ); + + migration.fix!(ctx); + + expect(readSource(ctx, 'src/Page.tsx')).toContain(`({ 'data-test': id }: { 'data-test': string })`); + }); + + it('parenthesizes a bare parameter it annotates', () => { + const ctx = reactProject(page(`const [present] = useIonModal(props => props.name as any, { name: 'Dave' });`)); + + migration.fix!(ctx); + + expect(readSource(ctx, 'src/Page.tsx')).toContain( + `useIonModal((props: { name: string }) => props.name as any, { name: 'Dave' })` + ); + }); + + it('leaves nothing for the report-only migration to say about a call it fixed', () => { + const ctx = reactProject(page(`const [present] = useIonModal(({ name }) => name as any, { name: 'Dave' });`)); + + migration.fix!(ctx); + + expect(reactOverlayHookPropsManual.detect(ctx)).toEqual([]); + }); + + it('runs before the report-only migration that covers the same calls', () => { + const selected = selectMigrations(allMigrations, { + fromMajor: 8, + toMajor: 9, + frameworks: ['react'], + includeExperimental: true, + }).map((selectedMigration) => selectedMigration.id); + + expect(selected.indexOf(migration.id)).toBeLessThan(selected.indexOf(reactOverlayHookPropsManual.id)); + }); + + it('declines a prop whose type is not in scope at the call', () => { + // The `Person` type is never imported here, so it prints as a module path and + // would not compile as an annotation. + const ctx = reactProject({ + 'src/internal.ts': `export interface Person { name: string }`, + 'src/api.ts': ` +import type { Person } from './internal'; +export const person: Person = { name: 'Dave' }; +`, + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; +import { person } from './api'; + +export const Page = () => { + const [present] = useIonModal(({ owner }) => owner as any, { owner: person }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('declines when the props passed do not cover what the component reads', () => { + // Annotating from `{ other: 1 }` would leave `name` unreadable on the new + // type: a different compile error, and one the report would no longer see. + const ctx = reactProject(page(`const [present] = useIonModal(({ name }) => name as any, { other: 1 });`)); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('declines a call that pins Props with a type argument', () => { + // The type argument is what the hook checks against, so an annotation + // derived from componentProps could contradict it. + const ctx = reactProject({ + 'src/Page.tsx': ` +import { useIonModal } from '@ionic/react'; + +interface Pinned { + name: number; +} + +export const Page = () => { + const [present] = useIonModal(({ name }) => name as any, { name: 'Dave' }); + return null as any; +}; +`, + }); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('declines an inline component that reads nothing', () => { + const ctx = reactProject(page(`const [present] = useIonModal((props) => 'literal' as any, { anything: 1 });`)); + + expect(migration.detect(ctx)).toEqual([]); + }); + + it('declines props spread from elsewhere', () => { + const ctx = reactProject( + page(` + const extra = { name: 'Dave' }; + const [present] = useIonModal(({ name }) => name as any, { ...extra });`) + ); + + expect(migration.detect(ctx)).toEqual([]); + }); +}); diff --git a/packages/migrate/test/react-typescript.test.ts b/packages/migrate/test/react-typescript.test.ts new file mode 100644 index 00000000000..693e3a5a476 --- /dev/null +++ b/packages/migrate/test/react-typescript.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it } from 'vitest'; + +import { createInMemoryContext } from '../src/context.js'; +import { selectMigrations } from '../src/registry.js'; +import { allMigrations } from '../src/migrations/index.js'; +import { reactTypescript } from '../src/migrations/v9/react-typescript.js'; + +describe('react-typescript', () => { + it('raises TypeScript to the 5.4 floor @ionic/react requires', () => { + const ctx = createInMemoryContext({ + 'package.json': JSON.stringify({ devDependencies: { typescript: '^4.9.5' } }, null, 2), + }); + + reactTypescript.fix!(ctx); + + expect(JSON.parse(ctx.readFile('package.json')!).devDependencies.typescript).toBe('^5.4.0'); + }); + + it('raises a pin declared in dependencies rather than devDependencies', () => { + const ctx = createInMemoryContext({ + 'package.json': JSON.stringify({ dependencies: { typescript: '~5.0.4' } }, null, 2), + }); + + reactTypescript.fix!(ctx); + + expect(JSON.parse(ctx.readFile('package.json')!).dependencies.typescript).toBe('^5.4.0'); + }); + + it('does not downgrade a pin already above the floor', () => { + const ctx = createInMemoryContext({ + 'package.json': JSON.stringify({ devDependencies: { typescript: '^5.9.2' } }, null, 2), + }); + + expect(reactTypescript.detect(ctx)).toEqual([]); + }); + + it('leaves a TypeScript 6 pin alone', () => { + // The caret target is a floor, not a ceiling, so a later major stays put. + const ctx = createInMemoryContext({ + 'package.json': JSON.stringify({ devDependencies: { typescript: '^6.0.0' } }, null, 2), + }); + + expect(reactTypescript.detect(ctx)).toEqual([]); + }); + + it('adds nothing to a project that does not use TypeScript', () => { + const ctx = createInMemoryContext({ + 'package.json': JSON.stringify({ dependencies: { react: '^18.0.0' } }, null, 2), + }); + + reactTypescript.fix!(ctx); + const pkg = JSON.parse(ctx.readFile('package.json')!); + + expect(reactTypescript.detect(ctx)).toEqual([]); + expect(pkg.devDependencies?.typescript).toBeUndefined(); + expect(pkg.dependencies.typescript).toBeUndefined(); + }); + + it('leaves a range it cannot parse alone', () => { + const ctx = createInMemoryContext({ + 'package.json': JSON.stringify({ devDependencies: { typescript: 'catalog:' } }, null, 2), + }); + + expect(reactTypescript.detect(ctx)).toEqual([]); + }); + + it('reports the change it would make', () => { + const ctx = createInMemoryContext({ + 'package.json': JSON.stringify({ devDependencies: { typescript: '^4.9.5' } }, null, 2), + }); + + expect(reactTypescript.detect(ctx)).toEqual([ + { filePath: 'package.json', line: 1, detail: 'set typescript to ^5.4.0' }, + ]); + }); + + it('is selected for a React project and not an Angular one', () => { + const selectedFor = (framework: 'react' | 'angular') => + selectMigrations(allMigrations, { fromMajor: 8, toMajor: 9, frameworks: [framework] }).map((m) => m.id); + + expect(selectedFor('react')).toContain('react-typescript'); + expect(selectedFor('angular')).not.toContain('react-typescript'); + }); +}); diff --git a/packages/react/src/framework-delegate.tsx b/packages/react/src/framework-delegate.tsx index 7379b4b97e0..e2952e9ba13 100644 --- a/packages/react/src/framework-delegate.tsx +++ b/packages/react/src/framework-delegate.tsx @@ -5,7 +5,7 @@ import { generateId } from './utils/generateId'; // TODO(FW-2959): types -type ReactComponent = (props?: any) => JSX.Element; +type ReactComponent = (props?: any) => React.ReactElement; export const ReactDelegate = ( addView: (view: React.ReactElement) => void, diff --git a/packages/react/src/hooks/__tests__/overlay-hook-types.spec.tsx b/packages/react/src/hooks/__tests__/overlay-hook-types.spec.tsx new file mode 100644 index 00000000000..f2cbcd79aa8 --- /dev/null +++ b/packages/react/src/hooks/__tests__/overlay-hook-types.spec.tsx @@ -0,0 +1,160 @@ +import type { + ComponentClass, + FC, + ForwardRefExoticComponent, + MemoExoticComponent, + ReactElement, + RefAttributes, +} from 'react'; + +import type { useIonModal } from '../useIonModal'; +import type { useIonPopover } from '../useIonPopover'; + +// The hooks are type-only imports, re-declared here. `@ionic/core/components` is ESM +// and Jest runs these specs as CommonJS, so importing them for real fails to load. +declare const useIonModalSignature: typeof useIonModal; +declare const useIonPopoverSignature: typeof useIonPopover; + +interface RequiredProps { + title: string; + count?: number; +} + +interface OptionalProps { + count?: number; +} + +declare const RequiredFunctionComponent: FC; +declare const RequiredClassComponent: ComponentClass; +declare const RequiredMemoComponent: MemoExoticComponent>; +declare const RequiredForwardRefComponent: ForwardRefExoticComponent>; +declare const OptionalPropsComponent: FC; +declare const NoPropsComponent: FC; +declare const DismissableComponent: FC<{ dismiss: (data: string, role: string) => void }>; +declare const UntypedComponent: FC; +declare const overlayElement: ReactElement; + +// None of these functions are invoked. They exist so `npm run typecheck` checks the +// calls inside them. +function componentPropsAreTypeChecked() { + useIonModalSignature(RequiredFunctionComponent, { title: 'Modal', count: 1 }); + useIonModalSignature(RequiredClassComponent, { title: 'Modal' }); + useIonModalSignature(RequiredMemoComponent, { title: 'Modal' }); + useIonModalSignature(RequiredForwardRefComponent, { title: 'Modal' }); + useIonPopoverSignature(RequiredFunctionComponent, { title: 'Popover', count: 1 }); + useIonPopoverSignature(RequiredClassComponent, { title: 'Popover' }); + + useIonPopoverSignature(RequiredMemoComponent, { title: 'Popover' }); + useIonPopoverSignature(RequiredForwardRefComponent, { title: 'Popover' }); + + // @ts-expect-error a required prop may not be omitted + useIonModalSignature(RequiredFunctionComponent, { count: 1 }); + // @ts-expect-error + useIonPopoverSignature(RequiredClassComponent, { count: 1 }); + + // @ts-expect-error unknown props are not accepted + useIonModalSignature(RequiredFunctionComponent, { title: 'Modal', unknown: true }); + // @ts-expect-error + useIonPopoverSignature(RequiredFunctionComponent, { title: 'Popover', unknown: true }); + + // @ts-expect-error props must match the declared types + useIonModalSignature(RequiredFunctionComponent, { title: 1 }); + // @ts-expect-error + useIonPopoverSignature(RequiredFunctionComponent, { title: 1 }); + + // @ts-expect-error a forwarded ref does not exempt a component from the check + useIonModalSignature(RequiredForwardRefComponent, { title: 1 }); +} + +// A rest-tuple signature still accepts an explicit type argument, so apps can pin the +// props type instead of relying on inference. +function explicitTypeArgumentsAreSupported() { + useIonModalSignature(RequiredFunctionComponent, { title: 'Modal' }); + useIonPopoverSignature(RequiredFunctionComponent, { title: 'Popover' }); + + // @ts-expect-error an explicit type argument still checks the props + useIonModalSignature(RequiredFunctionComponent, { count: 1 }); +} + +// Every call below omits `componentProps`, which `RequiredProps` does not allow. +function componentPropsAreRequiredWhenTheComponentRequiresThem() { + // @ts-expect-error + useIonModalSignature(RequiredFunctionComponent); + // @ts-expect-error + useIonModalSignature(RequiredClassComponent); + // @ts-expect-error + useIonModalSignature(RequiredMemoComponent); + // @ts-expect-error + useIonPopoverSignature(RequiredFunctionComponent); + // @ts-expect-error + useIonPopoverSignature(RequiredClassComponent); + // @ts-expect-error + useIonPopoverSignature(RequiredMemoComponent); +} + +// Untyped components keep the pre-v9 behavior, so upgrading apps don't get a new +// build error out of this. +function untypedComponentsStayPermissive() { + useIonModalSignature(UntypedComponent); + useIonModalSignature(UntypedComponent, { anything: true }); + useIonPopoverSignature(UntypedComponent); + useIonPopoverSignature(UntypedComponent, { anything: true }); +} + +function componentPropsAreOptionalWhenTheComponentHasNoRequiredProps() { + useIonModalSignature(NoPropsComponent); + useIonPopoverSignature(NoPropsComponent); + + useIonModalSignature(OptionalPropsComponent); + useIonModalSignature(OptionalPropsComponent, { count: 1 }); + useIonPopoverSignature(OptionalPropsComponent); + useIonPopoverSignature(OptionalPropsComponent, { count: 1 }); +} + +function jsxElementsRemainPermissive() { + useIonModalSignature(overlayElement); + useIonModalSignature(overlayElement, { anything: true }); + useIonPopoverSignature(overlayElement); + useIonPopoverSignature(overlayElement, { anything: true }); +} + +// Inline components need their props annotated, since `Props` is inferred from the +// component rather than from `componentProps`. See the `NoInfer` note in `useIonModal`. +function inlineComponentsAnnotateTheirProps() { + useIonModalSignature(({ name }: { name: string }) =>

Hello {name}.
, { name: 'Dave' }); + useIonPopoverSignature(({ name }: { name: string }) =>
Hello {name}.
, { name: 'Dave' }); +} + +// Overlays commonly pass `dismiss` back to the component through `componentProps`. +// That reads the binding the hook is still declaring, so it only compiles while +// `componentProps` stays out of inference. +function selfReferencingDismissCompiles() { + const [, dismissModal] = useIonModalSignature(DismissableComponent, { + dismiss: (data: string, role: string) => dismissModal(data, role), + }); + + const [, dismissPopover] = useIonPopoverSignature(DismissableComponent, { + dismiss: (data: string, role: string) => dismissPopover(data, role), + }); +} + +// Referenced so `noUnusedLocals` doesn't flag them. Keeping them unexported is what +// keeps the emitted declaration file empty. +void [ + componentPropsAreTypeChecked, + explicitTypeArgumentsAreSupported, + componentPropsAreRequiredWhenTheComponentRequiresThem, + untypedComponentsStayPermissive, + componentPropsAreOptionalWhenTheComponentHasNoRequiredProps, + jsxElementsRemainPermissive, + inlineComponentsAnnotateTheirProps, + selfReferencingDismissCompiles, +]; + +describe('overlay hook types', () => { + it('type checks component props at compile time', () => { + // The assertions in this file are enforced by `npm run typecheck`, which CI runs + // for this package. ts-jest sets `isolatedModules` in `tsconfig.spec.json` and so + // doesn't type check, which leaves nothing to assert at runtime. + }); +}); diff --git a/packages/react/src/hooks/useIonModal.ts b/packages/react/src/hooks/useIonModal.ts index aee3ea40e40..faa58df031b 100644 --- a/packages/react/src/hooks/useIonModal.ts +++ b/packages/react/src/hooks/useIonModal.ts @@ -1,6 +1,7 @@ import type { ModalOptions } from '@ionic/core/components'; import { modalController } from '@ionic/core/components'; import { defineCustomElement } from '@ionic/core/components/ion-modal.js'; +import type { ComponentType, ReactElement } from 'react'; import { useCallback } from 'react'; import type { ReactComponentOrElement } from '../models/ReactComponentOrElement'; @@ -10,12 +11,30 @@ import { useOverlay } from './useOverlay'; // TODO(FW-2959): types +// The `NoInfer` below keeps `componentProps` out of inference, so `Props` comes from +// the component alone. Overlays commonly pass `dismiss` back in through +// `componentProps`, and inferring from it would need the type of `dismiss` while that +// binding is still being declared, which TypeScript rejects as circular. The cost is +// that an inline component with no annotated props resolves to `{}`. + +/** + * A hook for presenting/dismissing an IonModal component + * @param component The component that the modal will show. Can be a React Component or a functional component + * @param componentProps The props that will be passed to the component. Required when the component declares required props + * @returns Returns the present and dismiss methods in an array + */ +export function useIonModal( + ...args: {} extends Props + ? [component: ComponentType, componentProps?: NoInfer] + : [component: ComponentType, componentProps: NoInfer] +): UseIonModalResult; /** * A hook for presenting/dismissing an IonModal component - * @param component The component that the modal will show. Can be a React Component, a functional component, or a JSX Element + * @param component A JSX Element that the modal will show. Props are already bound to the element, so `componentProps` is not type checked * @param componentProps The props that will be passed to the component, if required * @returns Returns the present and dismiss methods in an array */ +export function useIonModal(component: ReactElement, componentProps?: any): UseIonModalResult; export function useIonModal(component: ReactComponentOrElement, componentProps?: any): UseIonModalResult { const controller = useOverlay( 'IonModal', diff --git a/packages/react/src/hooks/useIonPopover.ts b/packages/react/src/hooks/useIonPopover.ts index c63df7b26e0..9ddd8b21609 100644 --- a/packages/react/src/hooks/useIonPopover.ts +++ b/packages/react/src/hooks/useIonPopover.ts @@ -1,6 +1,7 @@ import type { PopoverOptions } from '@ionic/core/components'; import { popoverController } from '@ionic/core/components'; import { defineCustomElement } from '@ionic/core/components/ion-popover.js'; +import type { ComponentType, ReactElement } from 'react'; import { useCallback } from 'react'; import type { ReactComponentOrElement } from '../models/ReactComponentOrElement'; @@ -10,12 +11,26 @@ import { useOverlay } from './useOverlay'; // TODO(FW-2959): types +// The `NoInfer` below does the same job as in `useIonModal`. See the note there. + +/** + * A hook for presenting/dismissing an IonPopover component + * @param component The component that the popover will show. Can be a React Component or a functional component + * @param componentProps The props that will be passed to the component. Required when the component declares required props + * @returns Returns the present and dismiss methods in an array + */ +export function useIonPopover( + ...args: {} extends Props + ? [component: ComponentType, componentProps?: NoInfer] + : [component: ComponentType, componentProps: NoInfer] +): UseIonPopoverResult; /** - * A hook for presenting/dismissing an IonPicker component - * @param component The component that the popover will show. Can be a React Component, a functional component, or a JSX Element + * A hook for presenting/dismissing an IonPopover component + * @param component A JSX Element that the popover will show. Props are already bound to the element, so `componentProps` is not type checked * @param componentProps The props that will be passed to the component, if required * @returns Returns the present and dismiss methods in an array */ +export function useIonPopover(component: ReactElement, componentProps?: any): UseIonPopoverResult; export function useIonPopover(component: ReactComponentOrElement, componentProps?: any): UseIonPopoverResult { const controller = useOverlay( 'IonPopover', diff --git a/packages/react/src/models/ReactComponentOrElement.ts b/packages/react/src/models/ReactComponentOrElement.ts index 2cb796ed837..798042c8c21 100644 --- a/packages/react/src/models/ReactComponentOrElement.ts +++ b/packages/react/src/models/ReactComponentOrElement.ts @@ -1,3 +1,3 @@ import type React from 'react'; -export type ReactComponentOrElement = React.ComponentClass | React.FC | JSX.Element; +export type ReactComponentOrElement = React.ComponentClass | React.FC | React.ReactElement; diff --git a/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx b/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx index fe3dfd0c284..f581d3e506a 100644 --- a/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx +++ b/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx @@ -51,7 +51,7 @@ const ModalHook: React.FC = () => { setCount(count + 1); }, [count, setCount]); - const handleDismissWithComponent = useCallback((data: any, role: string) => { + const handleDismissWithComponent = useCallback((data?: any, role?: string) => { dismissWithComponent(data, role); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -87,6 +87,9 @@ const ModalHook: React.FC = () => { const [presentSecondaryModal] = useIonModal(ModalSecondary); const [presentRootModal, dismissRootModal] = useIonModal(Body, { + type: 'Root', + count: count, + onIncrement: handleIncrement, onDismiss: () => { dismissRootModal(); presentSecondaryModal();