Skip to content

Document the MyTypes global namespace pattern#306

Open
narutamaaurum wants to merge 2 commits into
piotrwitek:masterfrom
narutamaaurum:fix-97-types-global-namespace
Open

Document the MyTypes global namespace pattern#306
narutamaaurum wants to merge 2 commits into
piotrwitek:masterfrom
narutamaaurum:fix-97-types-global-namespace

Conversation

@narutamaaurum

Copy link
Copy Markdown

Closes #97

Summary

  • add a dedicated README section for the MyTypes global namespace pattern
  • explain how modules.d.ts, store/types.d.ts, and services/types.d.ts work together
  • include example imports that show how shared app types are consumed

Validation

  • npm run ci-check
  • cd playground && npm run tsc

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new "Types global namespace" section to the documentation, detailing the use of the MyTypes module for shared ambient type declarations. Feedback suggests avoiding shorthand module declarations to prevent type safety issues, consolidating this new section with existing documentation to avoid redundancy, and refining type aliases to remove redundant wrappers and improve naming clarity.

Comment thread README.md Outdated
The `MyTypes` module is a shared namespace that lets each feature own its local
types while still contributing to a central app-level contract.

- `playground/typings/modules.d.ts` declares the ambient module name.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of a shorthand module declaration (e.g., declare module 'MyTypes';) in modules.d.ts is discouraged for internal type namespaces. Shorthand declarations make all imports from that module any, which bypasses type checking for missing or misspelled types. It is better to use an explicit (even if empty) module declaration: declare module 'MyTypes' {}.

Comment thread README_SOURCE.md Outdated
The `MyTypes` module is a shared namespace that lets each feature own its local
types while still contributing to a central app-level contract.

- `playground/typings/modules.d.ts` declares the ambient module name.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

As noted in the generated README, using a shorthand module declaration for MyTypes is unsafe as it defaults all imports to any. It is recommended to update the referenced playground/typings/modules.d.ts to use declare module 'MyTypes' {} instead.

Comment thread README.md

[⇧ back to top](#table-of-contents)

### Types global namespace

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This new section "Types global namespace" significantly overlaps with the existing "Create Global Store Types" section (around line 1288). To improve maintainability and avoid documentation drift, consider consolidating these sections or having one reference the other.

Comment thread README.md
import { StateType, ActionType } from 'typesafe-actions';

declare module 'MyTypes' {
export type Store = StateType<typeof import('./store').default>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The naming of the Store type alias is confusing because StateType<typeof store> returns the State type, not the Store instance type. Since RootState is already defined below, this alias might be redundant or should be renamed to reflect that it represents the state tree.

Comment thread README.md
declare module 'MyTypes' {
export type Store = StateType<typeof import('./store').default>;
export type RootAction = ActionType<typeof import('./root-action').default>;
export type RootState = StateType<ReturnType<typeof import('./root-reducer').default>>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of ReturnType inside StateType is redundant. StateType from typesafe-actions automatically extracts the return type when a function (like a reducer) is passed.

Suggested change
export type RootState = StateType<ReturnType<typeof import('./root-reducer').default>>;
export type RootState = StateType<typeof import('./root-reducer').default>;

Comment thread README_SOURCE.md

[⇧ back to top](#table-of-contents)

### Types global namespace

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This new section overlaps with the existing "Create Global Store Types" section at line 508. Consider merging them to maintain a single source of truth for the MyTypes pattern.

@narutamaaurum

Copy link
Copy Markdown
Author

Addressed the highest-priority namespace feedback in this branch update. The example now uses an explicit empty declare module 'MyTypes' {} declaration, and the new section points back to the earlier store-types setup to reduce duplication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a section about Types global namespace

1 participant