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

createStore Feature #22

Closed
wants to merge 2 commits into from

Conversation

sinster2003
Copy link
Contributor

Hi @lindelwa122 , so I have worked on this issue, the approach goes as follows:

  • To make the store global and access it from anywhere in the app I have used localStorage to store the object passed by the user
  • Then I have given functionality where the user cannot call the function twice if he does it throws an error
  • I have also worked on an exception where when the page is refreshed, it doesn't throw an error
  • I have returned the store in createStore alongwith updateState getState so that the store can use it.
  • if updateState has to be called i can simply return it with store by importing it but the point is I had to make a few changes in updateState to change the state of the store in the localStorage.
  • To retrieve store from localStorage, a new getStore function has to be created so that user can retrieve it from anywhere that object returned will contains variables,getState,updateState I will take up this issue after createStore

Go through the documentation once

@sinster2003
Copy link
Contributor Author

@lindelwa122 I will start working on getStore function if any improvements needed feel free.

@lindelwa122
Copy link
Owner

Hey, @sinster2003, I appreciate your implementation. Please make the following improvements before I can accept your PR:

  1. Ensure that private methods like _store are not exposed. Users should be able to access the store only through public methods like getState().
  2. I realize there might be a flaw in my initial logic while stating the function's requirements. If we remove the store when the page is reloaded, users may lose access to information stored prior to the reload. How about allowing users to invoke createStore as many times as they need? If the store already exists, the function should then add new variables to the store and overwrite existing ones with the same names. What are your thoughts on this change?

@lindelwa122 lindelwa122 mentioned this pull request Oct 5, 2023
@sinster2003
Copy link
Contributor Author

sinster2003 commented Oct 6, 2023

Hey @lindelwa122 I have figured out the changes that can be made ...

  1. I have handled the refresh or reload exception with the flag ensuring no data loss at all.
  2. createStore is used to just create store, the store won't be exposed the user can only retrieve the store for update and getState only using getStore()
  3. I had to implement getStore because of the this pointer in both getState and updateState we need an object reference of store.

Something like this:

createStore(store);

const data = getStore(); // gets the store from anywhere

console.log(data); // retrives store

console.log(data.getState("name")); // gets the property

const newdata = data.updateState("name", "akash"); // updates the state of store

console.log("New data", newdata); // new updated state
  1. I will be making a PR request commit soon

I think your previous thought process of createStore was absolutely right making it invokable only once and i feel that implementation is better than overwriting the store again and again, conceptually it doesn't sound good. If i give overwriting as the primary feature I just feel the importance of updateState will decrease multifolds. So we will proceed with the previous approach. I tested the changed code and it did work.
If changes needed do feel free.

@sinster2003
Copy link
Contributor Author

I will resolve the branch conflicts and make a PR

@sinster2003 sinster2003 closed this Oct 6, 2023
@sinster2003 sinster2003 deleted the createStoreFeature branch October 6, 2023 06:24
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.

None yet

2 participants