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

Code Refactored and modularized #9

Merged

Conversation

N3XT14
Copy link
Contributor

@N3XT14 N3XT14 commented Sep 29, 2024

This PR introduces significant refactoring and modularization to enhance code organization and maintainability. Key changes include:

Modularization of CLI Logic:

  • Extracted command handling logic into a dedicated utility file (cliUtils.ts) along with types, etc in their respective folder.

API Call Integration:

  • Added a layout for making REST API calls based on commands entered in the CLI.
  • This layout serves as a mock implementation to simulate interactions with the backend.

@N3XT14
Copy link
Contributor Author

N3XT14 commented Sep 29, 2024

@lucifercr07

Thanks to @rishavvajpayee a good portion of UI was already created considering what the objective of the PR was. For integrating with API. I am not completely sure about the URL and format if that's ready but I have created the wrapper which should make the later work easy and quick.

Let me know if something else needs to be done.

@N3XT14
Copy link
Contributor Author

N3XT14 commented Sep 30, 2024

@lucifercr07

I have integrated the CLI API.
Once Backend is up all we need to do is change the constant file apiEndpoints.ts

There were 2 issues I observed which I am describing here regarding playground-mono.

  1. Handling of CORS
  2. Handling of headers such as setting content-type: applicaton/json. I believe we should check on server side about this or else we should throw errors when headers are set from frontend

// This folder will contain or hold constants. For example in this particular file we can store the api endpoints as constants
export const CLI_COMMAND_URL="http://localhost:8080/cli"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better if this derived via config/env var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. In fact NextJS has built-in support for that, So we can have .env, .env.development, and so on to handle them.

@lucifercr07
Copy link
Contributor

@N3XT14 please resolve conflicts.

@N3XT14 N3XT14 force-pushed the feature/CLI-for-dicedb-playground-ui branch from 61f6d80 to 73f665d Compare October 1, 2024 04:04
@N3XT14
Copy link
Contributor Author

N3XT14 commented Oct 1, 2024

@lucifercr07 . I have resolved the conflicts

package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this btw?

@lucifercr07 lucifercr07 merged commit bd8499c into DiceDB:master Oct 1, 2024
1 check passed
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.

3 participants