fix: replace @ts-ignore with Metro-specific type definitions#832
Open
YevheniiKotyrlo wants to merge 4 commits intostorybookjs:nextfrom
Open
fix: replace @ts-ignore with Metro-specific type definitions#832YevheniiKotyrlo wants to merge 4 commits intostorybookjs:nextfrom
YevheniiKotyrlo wants to merge 4 commits intostorybookjs:nextfrom
Conversation
The generated `storybook.requires.ts` uses Metro's `require.context` and `module.hot` APIs, which are copied from webpack. Adding `@types/webpack-env` as a dependency provides proper TypeScript definitions for consumers. This follows the precedent established in commit 36ee762: > "@types/webpack-env must be defined as dependencies in packages that > export a type/function using typings from @types/webpack-env" Since @storybook/react-native generates code that consumers compile, and that code uses require.context/module.hot, this qualifies as "exporting" code dependent on these types. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
|
Hey thanks for this PR, however I wouldn't want to add this dependency personally. I understand the intention behind this pr, however I don't think this is the right fix since the metro implementation doesn't match the webpack one and we shouldn't rely on any dependency to webpack in my opinion. If anything we should copy the type definitions from expo here However its also low priority in my opinion since this ts-ignore exists in a generated file that can be ignored in eslint |
Replace @types/webpack-env dependency with custom Metro type definitions, following the same approach used by Expo. Metro's require.context API intentionally matches webpack's API by design, but using webpack types caused concerns about API differences. This change: - Adds metro-env.d.ts with Metro-specific types in __MetroModuleApi namespace - Exports types via @storybook/react-native/metro-env - Auto-injects reference directive in generated storybook.requires.ts files - Removes @types/webpack-env dependency References: - Expo's approach: https://github.com/expo/expo/blob/main/packages/expo/types/metro-require.d.ts - Metro require.context PR: facebook/metro#822 - Metro HMR Flow types: https://github.com/facebook/metro/blob/main/packages/metro-runtime/src/polyfills/require.js.flow
YevheniiKotyrlo
changed the title
- Remove unnecessary 'declare var' statements (module/require already declared by Node types) - Add eslint-disable for intentional empty interface (declaration merging pattern)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes
@ts-ignorecomment from generatedstorybook.requires.tsby providing proper Metro type definitions.Previous approach (rejected): Added
@types/webpack-envas dependency.New approach (per maintainer feedback): Custom Metro type definitions following the same pattern used by Expo.
Background Research
Metro's
require.contextAPI intentionally matches webpack's API by design (Metro PR #822), but the runtime implementations differ. Rather than using webpack types, this PR provides Metro-specific types.How Expo handles this:
packages/expo/types/metro-require.d.ts__MetroModuleApinamespace to avoid conflicts with other type definitionsWhy not auto-convert from Flow?
Changes
metro-env.d.ts- New file with Metro type definitions__MetroModuleApi.RequireContext- require.context() return type__MetroModuleApi.RequireFunction- require.context() method signature__MetroModuleApi.Hot- module.hot HMR APINodeJS.RequireandNodeModuleinterfacespackage.json@types/webpack-envdependency"./metro-env": "./metro-env.d.ts"exportscripts/generate.js/// <reference types="@storybook/react-native/metro-env" />to generated TypeScript filesReferences
Test Plan
yarn test:generatepasses@ts-ignoreneeded in generated code