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

refactor: Replace JSON.parse with parseJsonWithSchema (@dev-mohit06) #6207

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dev-mohit06
Copy link

@dev-mohit06 dev-mohit06 commented Jan 23, 2025

Description

Replaces raw JSON parsing with schema-based validation across frontend TypeScript files to improve type safety and error handling.

Scope of Changes

  • Updated JSON parsing in:

    • account.ts
    • import-export-settings.ts
    • analytics-controller.ts
    • local-storage-with-schema.ts
    • url-handler.ts
    • commandline/lists.ts
    • test/wikipedia.ts
  • Added schema in test/custom-text.ts:

    export const customTextDataSchema = z.object({
      text: z.array(z.string()),
      mode: CustomTextModeSchema,
      limit: z.object({ 
        value: z.number(), 
        mode: CustomTextLimitModeSchema 
      }),
      pipeDelimiter: z.boolean(),
    });

Benefits

  • Enhanced runtime type safety
  • More robust error handling
  • Consistent JSON parsing approach

Checks

  • Follows Conventional Commits
  • Includes GitHub username
  • Adding quotes? (N/A)
  • Adding language/theme? (N/A)

@monkeytypegeorge monkeytypegeorge added the frontend User interface or web stuff label Jan 23, 2025
Copy link
Member

@fehmer fehmer left a comment

Choose a reason for hiding this comment

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

Hi @dev-mohit06, nice work. I added some comments for you.

@github-actions github-actions bot added the waiting for update Pull requests or issues that require changes/comments before continuing label Jan 23, 2025
@github-actions github-actions bot removed the waiting for update Pull requests or issues that require changes/comments before continuing label Jan 23, 2025
…WithSchema (@dev-mohit06)

- Updated schema definitions for better clarity and reusability.
- Replaced  with  where applicable.
- Addressed all review comments for better code consistency.
@dev-mohit06
Copy link
Author

Hey @fehmer, could you take a look at the changes?

frontend/src/ts/controllers/analytics-controller.ts Outdated Show resolved Hide resolved
frontend/src/ts/controllers/analytics-controller.ts Outdated Show resolved Hide resolved
frontend/src/ts/event-handlers/account.ts Outdated Show resolved Hide resolved
frontend/src/ts/test/wikipedia.ts Outdated Show resolved Hide resolved
frontend/src/ts/utils/url-handler.ts Outdated Show resolved Hide resolved
frontend/src/ts/utils/url-handler.ts Outdated Show resolved Hide resolved
frontend/src/ts/utils/url-handler.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added waiting for update Pull requests or issues that require changes/comments before continuing and removed waiting for update Pull requests or issues that require changes/comments before continuing labels Jan 24, 2025
@dev-mohit06
Copy link
Author

Hey @fehmer, could you take a look at the changes?

@dev-mohit06 dev-mohit06 requested a review from fehmer January 24, 2025 12:33
@github-actions github-actions bot added the waiting for review Pull requests that require a review before continuing label Jan 24, 2025
fehmer
fehmer previously approved these changes Jan 24, 2025
Copy link
Member

@fehmer fehmer left a comment

Choose a reason for hiding this comment

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

Hi @dev-mohit06 , changes look good. Please remove the code comment on the unresolved PR comment or replace with the suggested comments.

Thank you for your contribution. Do you want to take the backend part next?

@github-actions github-actions bot removed the waiting for review Pull requests that require a review before continuing label Jan 24, 2025
@dev-mohit06
Copy link
Author

Hi @fehmer,

Thank you for the feedback! I’ll address the code comment as suggested. Regarding the backend part, I’d be happy to take it up. Could you please share the details or scope of the task?

@dev-mohit06 dev-mohit06 requested a review from fehmer January 25, 2025 11:47
@github-actions github-actions bot added the waiting for review Pull requests that require a review before continuing label Jan 25, 2025
@github-actions github-actions bot removed the waiting for review Pull requests that require a review before continuing label Jan 25, 2025
@fehmer
Copy link
Member

fehmer commented Jan 25, 2025

Hi @fehmer,

Thank you for the feedback! I’ll address the code comment as suggested. Regarding the backend part, I’d be happy to take it up. Could you please share the details or scope of the task?

Same as for the frontend for the files services/weekly-xp-leaderboard.ts and utils/daily-leaderboards.ts

@dev-mohit06
Copy link
Author

Hi @fehmer, thank you for the feedback! I’ll address the code comment and work on the backend implementation as well. By the way, could you let me know when this PR will be merged?

@dev-mohit06
Copy link
Author

Hi @fehmer, just wanted to follow up regarding the PR. Let me know if there’s anything else needed from my side or an update on the merge timeline. Thanks!

@dev-mohit06
Copy link
Author

@Miodec could you tell me what should i need to do next?

@dev-mohit06
Copy link
Author

dev-mohit06 commented Jan 27, 2025

@fehmer @Miodec can you guys reply?

@fehmer
Copy link
Member

fehmer commented Jan 27, 2025

hi @dev-mohit06 , your PR looks good from my side. next mio will review and merge when he has time. You don't need to follow up or rebase unless requested. Thanks for your contribution

@dev-mohit06
Copy link
Author

@fehmer, you mentioned the backend file changes. I’ve completed those updates. Should I create the PR now?

@dev-mohit06
Copy link
Author

@Miodec Could you please merge this branch? It has already been approved by @fehmer. Thank you!

@Miodec
Copy link
Member

Miodec commented Jan 29, 2025

Ive been sick with some kind of flu lately. Please just be patient, Ill get to all PRs eventually.

@dev-mohit06
Copy link
Author

Ive been sick with some kind of flu lately. Please just be patient, Ill get to all PRs eventually.

@Miodec No worries! Take your time and focus on getting better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend User interface or web stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants