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

Tests+Docs+Examples+Error Messages - after #28 #27

Merged
merged 25 commits into from Jun 3, 2019

Conversation

Projects
None yet
2 participants
@adamkleingit
Copy link
Collaborator

commented May 27, 2019

Sorry for the long PR...

@adamkleingit adamkleingit requested a review from morsdyce May 27, 2019

@adamkleingit adamkleingit changed the title Tests+Docs+Examples+Error Messages Tests+Docs+Examples+Error Messages - after #28 May 28, 2019

- Time Travelling
- TypeScript Support
- Better error messages
Go here:

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

I'd keep some parts of the readme:
What, why and Context

This comment has been minimized.

Copy link
@adamkleingit

adamkleingit May 28, 2019

Author Collaborator

Why not let them read it in the docs?

This comment has been minimized.

Copy link
@adamkleingit

adamkleingit May 28, 2019

Author Collaborator

Also, I kept the tl;dr so that people read the code and get intrigued and look at the docs

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

People won't click unless there's something that tells them what it is

it('should not allow to create a unit twice', () => {
const fn = () => {};
store.createUnit(fn);
expect(() => store.createUnit(fn));

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

what is the assertion here?

expect(() => store.createUnit(fn));
});
it('should allow to unsubscribe from subscribing to creating a unit', () => {
expect.assertions(0);

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

why not pass a jest spy to store.onUnitsChanged and then expect(spy).not.toHaveBeenCalled ?

});
unit.run();
unit.notify();
expect(result).toBe(value);

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

instead of result you can pass a spy and then expect(spy).toHaveBeenCalledWith(value)

expect(result).toBe(value);
});
it('should allow to unsubscribe to value change', () => {
expect.assertions(0);

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

same as previous comment regarding 0 assertions

it('should allow to define a reusable unit', () => {
const useSomething = reusable(() => useState(0));

expect(typeof useSomething).toBe('function');

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

expect(useSomething).toBe(expect.any(Function))

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

I'm not sure how much this test is helpful anyway, you are already testing it in the next test should return a unit value if it wasn't a function that test will fail

wrapper: ReusableProvider
});

act(() => result.current[1](3));

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

this is very unreadable, maybe at least create constants to make it more clear?
const SETTER_FN = 1;
const VALUE = 0;

result.currentSETTER_FN
and
result.current[VALUE]

} = require("customize-cra");

module.exports = override(
addBabelPlugin('babel-plugin-reusable')

This comment has been minimized.

Copy link
@morsdyce

morsdyce May 28, 2019

Collaborator

we can probably delete this now

This comment has been minimized.

Copy link
@adamkleingit

adamkleingit May 28, 2019

Author Collaborator

I think we will need a plugin later on for the unit's names, so I'll just leave the skeleton

Fixed Maayan comments - using spies, fixed expect to throw, more read…
…ble hook tests, removed babel plugin from example
@adamkleingit

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2019

Fixed all but Readme

@morsdyce morsdyce merged commit 179233e into master Jun 3, 2019

@morsdyce morsdyce deleted the tests branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.