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

[v2] Web Storage #216

Merged
merged 13 commits into from
Oct 20, 2019
Merged

Conversation

JesseRWeigel
Copy link

Summary:

#52

I added a web-storage option that uses localStorage(default) or sessionStorage(by passing true as the first param when creating a Webstorage instance).

I also added a react app as an example to demonstrate using Webstorage. I followed the same UI structure as the React Native example app.

Test Plan:

You can test Webstorage by running yarn start from the examples/web folder. To use sessionStorage instead of localStorage, pass in true as the first parameter to WebStorage() in storage.js in the web folder.
const web = new WebStorage(true);

There are still some things that should be done:

  • Write tests
  • Write documentation

Also, I am not very familiar with Typescript, so I expect that there may be some issues with how I implemented it.

Copy link
Member

@krizzu krizzu left a comment

Choose a reason for hiding this comment

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

So far so good - left few commnets. I'm going to revisit this PR later today

packages/storage-web/src/index.ts Outdated Show resolved Hide resolved
examples/web/README.md Outdated Show resolved Hide resolved
examples/web/package.json Outdated Show resolved Hide resolved
examples/web/src/App.js Outdated Show resolved Hide resolved
@krizzu
Copy link
Member

krizzu commented Sep 26, 2019

@JesseRWeigel On the second thought, it's a lot to tackle at once: Storage, Example and Docs. Let's keep this PR with only Storage implementation and tests for it - let's leave other two for separate PRs.

@JesseRWeigel
Copy link
Author

@krizzu Good idea! I will keep everything in separate PRs.

@krizzu krizzu changed the title feat: add option to use localStorage or sessionStorage [v2] Web Storage Sep 27, 2019
@JesseRWeigel
Copy link
Author

@krizzu I removed the example app and finished the unit tests for local and session storage. Everything is working, but I have a few type errors that I haven't been able to resolve. Any assistance with the type errors that you could provide would be greatly appreciated.

packages/storage-web/__tests__/index.test.ts:34:35 - error TS2741: Property 'key2' is missing in type '{ key1: string; }' but required in type '{ key1: any; key2: any; }'.

34         await webStorage.setMany([{key1: 'value1'}, {key2: 'value2'}]);
                                     ~~~~~~~~~~~~~~~~

packages/storage-web/__tests__/index.test.ts:34:53 - error TS2741: Property 'key1' is missing in type '{ key2: string; }' but required in type '{ key1: any; key2: any; }'.

34         await webStorage.setMany([{key1: 'value1'}, {key2: 'value2'}]);
                                                       ~~~~~~~~~~~~~~~~

packages/storage-web/src/index.ts:53:5 - error TS2322: Type 'any[]' is not assignable to type '{ [k in K]: T[k] | null; }'.

53     return Promise.all(keys.map(k => this.storage.getItem(k)));
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

packages/storage-web/src/index.ts:68:33 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ [k in K]: T[k]; }'.
  No index signature with a parameter of type 'string' was found on type '{ [k in K]: T[k]; }'.

68       this.storage.setItem(key, keyValue[key]);

It seems like the first two type errors are related to the third one, and would be resolved along with it.

Copy link
Member

@krizzu krizzu left a comment

Choose a reason for hiding this comment

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

Good work so far 💪

Your issues comes from the wrongly typed Storage - I've left comments to help you find them.

Also, I see that we need to change TS settings - each package should have its own tsconfig, extending the main one. I'm going to fix this one once you're done with requested changes.

packages/core/src/defaults.ts Outdated Show resolved Hide resolved
packages/storage-web/__tests__/index.test.ts Outdated Show resolved Hide resolved
packages/storage-web/__tests__/index.test.ts Outdated Show resolved Hide resolved
packages/storage-web/src/index.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
packages/storage-web/src/index.ts Outdated Show resolved Hide resolved
packages/storage-web/src/index.ts Outdated Show resolved Hide resolved
@krizzu
Copy link
Member

krizzu commented Oct 12, 2019

@JesseRWeigel Sorry for late response.

I've pushed a commit, that includes few changes to packages - this makes each of them an independent projects.

What's left to do here are typing of Storage.

Copy link
Member

@krizzu krizzu left a comment

Choose a reason for hiding this comment

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

We're almost there 💪
Please pull the changes I committed and look through comments I left for you.

If you need more help, just ask

packages/storage-web/src/index.ts Outdated Show resolved Hide resolved
packages/storage-web/src/index.ts Outdated Show resolved Hide resolved
@krizzu krizzu added the v2 label Oct 13, 2019
@krizzu krizzu merged commit 248ec89 into react-native-async-storage:master Oct 20, 2019
@krizzu
Copy link
Member

krizzu commented Oct 20, 2019

@JesseRWeigel Great work, thanks a lot!

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