Skip to content

refactor : convert entity-link component from JSX to typescript#1230

Open
garvthakre wants to merge 2 commits intometabrainz:masterfrom
garvthakre:Convert-entity-link-to-typescript
Open

refactor : convert entity-link component from JSX to typescript#1230
garvthakre wants to merge 2 commits intometabrainz:masterfrom
garvthakre:Convert-entity-link-to-typescript

Conversation

@garvthakre
Copy link
Contributor

Problem

This PR is part of the ongoing TypeScript migration of the BookBrainz codebase. The entity-link component is currently written in JavaScript with PropTypes for type checking, which provides less type safety compared to TypeScript's compile-time type checking

Solution

1.Renaming the file from .js to .tsx
2.Maintaining all existing functionality without any behavioral changes

Areas of Impact

File affected: src/client/components/entity-link.js → src/client/components/entity-link.tsx

@garvthakre
Copy link
Contributor Author

hey @MonkeyDo , can u also close this PR's. whenever u got the time. means a lot :)

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Small type change to avoid duplication, and we're good.

Generally, it's a good idea to check in the codebase if a type or function already exists, although I will admit in this case it might be hard ot find with such a generic name that appears in a lot of places (although a codebase-wide search for type entity would have worked)

Comment on lines 23 to 30
type Entity = {
bbid: string;
type: string;
defaultAlias?: {
name: string;
};
disambiguation?: string;
};
Copy link
Member

Choose a reason for hiding this comment

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

There is already an existing Entity type you can use, although you will have to add the disambiguation field to it:

export type Entity = {
bbid: string,
defaultAlias?: {
name: string
},
type: EntityType
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MonkeyDo really sorry for this small errors, and yeah really thanks for that search suggestion, i think this will help me alot ...

@garvthakre
Copy link
Contributor Author

@MonkeyDo , I've updated the PR to use the existing Entity type from relationship-editor/types.ts and added the disambiguation field to it instead of creating a duplicate type.

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

Comments