[v2] Initial Core implementation #136

krizzu · 2019-06-22T10:42:35Z

Summary:

Todo:

Initial implementation
Write tests
CI to run tests

thymikee · 2019-07-01T17:01:06Z

.circleci/config.yml

          name: Type check
          command: yarn test:types

+  "Test: unit":


FYI future notice: use the community orb for easier circle management: https://github.com/react-native-community/react-native-circleci-orb

thymikee · 2019-07-01T17:01:46Z

babel.config.js

+      '@babel/preset-env',
+      {
+        targets: {
+          node: 'current',


Suggested change

node: 'current',

node: 8.3,

RN requires Node 8.3+, we should be compatible

thymikee · 2019-07-01T17:06:08Z

core/__tests__/AsyncStorage.test.ts

+  });
+
+  describe('main API', () => {
+    it('handles basic set/read/remove calls', async () => {


I'd use test.each for these tests for convenience

thymikee · 2019-07-01T17:06:31Z

core/__tests__/AsyncStorage.test.ts

+        name: 'Jerry',
+      };
+
+      // get


these comments are useless, please remove them all

oops 🤦‍♂

thymikee · 2019-07-01T17:09:18Z

core/__tests__/AsyncStorage.test.ts

+      });
+
+      // instance
+      expect(asyncStorage.instance()).toBe(mockedStorage);


is this a part of your public API? asyncStorage is already an instance of a class, so this is quite unintuitive

Yes, it is. Some storages might come with other functionalities than AsyncStorage requires, so instance gives direct access to the storage (and to said functionalities). I guess I can rename it to getStorageInstance or smth

Hm, this is dangerous. All functionalities should be behind a facade of your API, don't you think?

That'd be ideal, but we cannot predict all possibilities, right? Besides, I can imagine that'd be used only in advanced cases, which normally you wouldn't need.

Those extra methods would be expected to be documented in storage's docs.

I'd be cautious about that, this is what Enzyme does that sooner or later made everyone using this API because guides and blog post were made. I'd remove this and think about expanding API when necessary

Going to leave it like that for now, with a note to re-think it later.

thymikee · 2019-07-01T17:09:53Z

core/__tests__/AsyncStorage.test.ts

+        });
+
+        await as.get('key');
+        expect(loggerFunc).toBeCalledTimes(1);


called with what?

thymikee · 2019-07-01T17:10:45Z

core/__tests__/AsyncStorage.test.ts

+        await as.get('key');
+
+        expect(errorHandler).toBeCalledTimes(1);
+        expect(errorHandler.mock.calls[0][0]).toBe(error);


Suggested change

expect(errorHandler.mock.calls[0][0]).toBe(error);

expect(errorHandler).toHaveBeenCalledWith(error);

thymikee · 2019-07-01T17:12:16Z

core/__tests__/core.test.ts

+
+describe('AsyncStorageFactory', () => {
+  it('Throws when tried to instantiate', () => {
+    expect(() => new Factory()).toThrow();


please add some text that you expect to throw, it may have thrown for other reasons and test will pass, something like:

Suggested change

expect(() => new Factory()).toThrow();

expect(() => new Factory()).toThrow('forbidden to instantiate Factory');

thymikee · 2019-07-01T17:21:47Z

core/__tests__/core.test.ts

+    simpleErrorHandler(errorMessage);
+
+    expect(console.error).toBeCalledTimes(1);
+    expect(console.error.mock.calls[0][0]).toEqual(errorMessage);


Suggested change

expect(console.error.mock.calls[0][0]).toEqual(errorMessage);

expect(console.error).toHaveBeenCalledWith(errorMessage);

thymikee · 2019-07-01T17:34:23Z

core/types/index.d.ts

+ * Core Async Storage API
+ *
+ */
+declare class AsyncStorage<


can't these types be output by tsc?

They were at first, but then I decided to remove it, as I needed more control over it

thymikee · 2019-07-01T17:36:28Z

jest.config.js

@@ -0,0 +1,19 @@
+const commonSettings = {
+  transform: {
+    '^.+\\.tsx?$': 'babel-jest',


this is the default transform, you should be good to remove it

krizzu · 2019-07-02T15:36:58Z

Thanks a lot!

thymikee · 2019-07-02T15:39:52Z

core/__tests__/AsyncStorage.test.ts

+        };
+
+        switch (methodName) {
+          case 'get': {


Yikes, that's not what I meant, this code is almost no different. You can use each like this:

it.each([ ['get', 'myKey'], ['set', 'myKey', {name: 'Jerry'}], ['remove', 'myKey'] ])('handles single %s api call', async (methodName: 'get' | 'set' | 'remove', ...args) => { await as[methodName](...args); expect(mockedStorage.getSingle).toBeCalledWith(...args.concat(null)); }

yh, shame, this looks better. Will cover this in next PR.

krizzu added v2 labels Jun 22, 2019

feat: initial implementation

fd36780

krizzu force-pushed the core-impl branch from 29aeee5 to fd36780 Compare June 26, 2019 05:53

Krzysztof Borowy added 2 commits June 26, 2019 08:01

fix: correct logger/error funcs

967bdc9

tests: add unit tests

24cae3c

krizzu force-pushed the core-impl branch from 549957b to 24cae3c Compare June 29, 2019 11:23

chore: temp disable lint for storages, ci unit tests

4957093

krizzu force-pushed the core-impl branch from 3c403ad to 4957093 Compare June 29, 2019 11:29

thymikee reviewed Jul 1, 2019

View reviewed changes

tests: improve tests

d677358

krizzu force-pushed the core-impl branch from 1420a2a to d677358 Compare July 2, 2019 15:33

krizzu merged commit eeb7560 into master Jul 2, 2019

krizzu deleted the core-impl branch July 2, 2019 15:36

krizzu removed the WIP label Jul 2, 2019

thymikee reviewed Jul 2, 2019

View reviewed changes

	expect(errorHandler.mock.calls[0][0]).toBe(error);
	expect(errorHandler).toHaveBeenCalledWith(error);

	expect(() => new Factory()).toThrow();
	expect(() => new Factory()).toThrow('forbidden to instantiate Factory');

	expect(console.error.mock.calls[0][0]).toEqual(errorMessage);
	expect(console.error).toHaveBeenCalledWith(errorMessage);

[v2] Initial Core implementation #136

[v2] Initial Core implementation #136

Uh oh!

Conversation

krizzu commented Jun 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thymikee Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krizzu Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krizzu Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krizzu commented Jul 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krizzu Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krizzu commented Jun 22, 2019 •

edited

Loading

thymikee Jul 1, 2019 •

edited

Loading

krizzu Jul 1, 2019 •

edited

Loading

krizzu Jul 1, 2019 •

edited

Loading

krizzu Jul 2, 2019 •

edited

Loading