Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change MojoModels from type to interface #166

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

dmanto
Copy link
Contributor

@dmanto dmanto commented Jan 15, 2025

Description

This PR updates the MojoModels definition from a type to an interface to enable declaration merging. This change allows developers to extend MojoModels in their applications, similar to how MojoContext can be extended.

Declaration merging is a key TypeScript feature that enhances extensibility, making it easier for developers to define and use typed models within their apps.

Changes Made

  1. Updated src/types.ts file:

Replaced

type MojoModels = Record<string, any>;

with:

export interface MojoModels {
  [key: string]: any;
}
  1. Modified an Existing Test File:
    Added a declaration merging example in the existing test file ./test/support/ts/full-app/src/index.ts. Here's how the declare module portion looks:
declare module '../../../../../lib/core.js' {
  interface MojoModels {
    bar: Bar;
  }
}

This demonstrates how developers can extend MojoModels and assign strongly typed properties like bar. It effectively verifies that the updated MojoModels supports declaration merging, as reverting to a type causes the test file to fail during the build process.

Benefits

Enables developers to extend MojoModels via declaration merging, improving type safety and flexibility.
Provides an example in the test file to illustrate usage, making it easier for others to adopt this pattern.

Considerations

  1. The modification to the test file is not a "formal" TypeScript test but serves as an effective demonstration of declaration merging in action. This change can be revised or removed based on feedback.
  2. ESLint's @typescript-eslint/consistent-indexed-object-style rule was disabled inline for MojoModels since it enforces Record over interface.

@kraih
Copy link
Member

kraih commented Jan 16, 2025

Thanks, looks like a very reasonable change. Wonder if we should just disable the eslint rule though, manual disabling looks ugly, and i'm not sure how useful it actually is for us. 🤔

@dmanto
Copy link
Contributor Author

dmanto commented Jan 16, 2025

Just checked that adding '@typescript-eslint/consistent-indexed-object-style': 'off' to the rules array in eslint.config.js disables the rule globally, and everything looks perfect after that. The rule doesn’t seem particularly useful outside of very specific cases, and in this context, where interface is required for declaration merging, it feels counterproductive.

Let me know if you'd like me to include this change as part of the PR. Also, in that case, let me know if you’d like me to squash the two commits.

@kraih
Copy link
Member

kraih commented Jan 17, 2025

Yes, please include the change and squash.

@dmanto dmanto force-pushed the change-MojoModels-to-interface branch from c7de533 to 312a2b0 Compare January 17, 2025 12:29
@dmanto
Copy link
Contributor Author

dmanto commented Feb 4, 2025

Just checking in on this—let me know if there's anything else you'd like me to adjust.

@kraih kraih merged commit 517bad1 into mojolicious:main Feb 5, 2025
10 checks passed
@kraih
Copy link
Member

kraih commented Feb 5, 2025

Sorry, i just forgot.

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.

2 participants