Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Namespaces #17
Namespaces #17
Changes from all commits
4040b98
b8951b6
95078bc
61d63d1
e444957
e0fe8f4
b3614b6
dd0cb26
69ff5f0
a02c78b
bfc96ae
30c7ca8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if
meta
contains anamespace
key? Maybenamespace
should be top level? Unless there is a reason for it to live undermeta
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tooling controls
meta
. Our action creators doesn't give the call sites access tometa
.FSA defines
type
,payload
,meta
anderror
as the only allowed top level properties.In short, I think this is a non-issue, and we have a test verifying that if it should happen, the new namespace takes precedence. This is also documented in
namespaceActionCreator
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't actually wait for async things to finish in tests. In this case it might not matter much, but I'm sure there are facilities for testing asynchronous things synchronously in tests (i.e. by providing a mock that returns when triggered explicitly).
More importantly though, I don't think we should be testing observables by converting them to promises. I did a quick google search, and this came up (more info here).
I think it's worth investing some time into thinking how we're going to test observables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AVA is a testing framework with inbuilt support for observables and running tests concurrently. It also works with the aforementioned rxjs-marbles lib.
Looks pretty cool.
I guess you're not too hot for tests and testing frameworks, but I'll just leave this comment here anyway as a note to self.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to learn more about testing frameworks :)
@anton said he would look into marble testing.
My experience with writing these tests are that the awaits we are using here will only wait for one node tick, and the tests using them does prove something worthwhile about the code. I'm all open for a better way to test them, though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think testing strategy and frameworks is out of scope for this PR. Please mention AVA in #14