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

Namespaces #17

Merged
merged 12 commits into from
Sep 27, 2019
Merged

Namespaces #17

merged 12 commits into from
Sep 27, 2019

Conversation

tlaundal
Copy link
Contributor

@tlaundal tlaundal commented Sep 20, 2019

Adds tooling for namespacing actions so actions from multiple instances of the same "thing" can be separated.

This all works by looking at a namespace property in the meta field of the actions. There are two possible ways to use it:

  1. Namespacing an action
    i. Create a namespace (using createNamespace)
    ii. Create a namespaced version of an action (using namespaceActionCreator)
    iii. Use the namespaced action as usual to create action objects and dispatch them on the action$
    iv. Use the filterNamespace streaming operator to filter the action$ for only that namespace
  2. Namespacing an action dispatcher
    i. Create a namespace
    ii. Create a namespaced version of an action dispatcher
    iii. Use the namespaced action dispatcher as normal to dispatch actions to the action$
    iv. Use the filterNamespace streaming operator to filter the action$ for only that namespace

@tlaundal tlaundal added the work-in-progress Work in progress label Sep 20, 2019
@tlaundal tlaundal self-assigned this Sep 20, 2019
@tlaundal tlaundal changed the title Qualifiers Namespaces Sep 24, 2019
@tlaundal tlaundal removed the work-in-progress Work in progress label Sep 24, 2019
src/operators/operators.ts Outdated Show resolved Hide resolved
examples/namespace.tests.ts Outdated Show resolved Hide resolved
src/internal/testUtils.ts Outdated Show resolved Hide resolved
src/internal/testUtils.ts Outdated Show resolved Hide resolved
src/namespace.tests.ts Outdated Show resolved Hide resolved
src/namespace.ts Outdated Show resolved Hide resolved
src/namespace.ts Show resolved Hide resolved
src/namespace.ts Show resolved Hide resolved
Copy link
Contributor Author

@tlaundal tlaundal left a comment

Choose a reason for hiding this comment

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

All comments so far resolved. Will you do final review @chriskr and @anton164 ?

src/namespace.ts Show resolved Hide resolved
examples/namespace.tests.ts Show resolved Hide resolved
examples/namespace.tests.ts Outdated Show resolved Hide resolved
src/operators/operators.tests.ts Show resolved Hide resolved
payload: action.payload,
meta: {
...action.meta,
namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if meta contains a namespace key? Maybe namespace should be top level? Unless there is a reason for it to live under meta.

Copy link
Contributor Author

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 to meta.
FSA defines type, payload, meta and error 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.

@@ -35,16 +36,15 @@ describe('operators', function() {
const targetType = 'Correct type';
const otherType = 'Wrong type';

await of<ActionWithoutPayload>(
const res = await of<ActionWithoutPayload>(
Copy link
Contributor

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.

Copy link
Contributor

@holographic-principle holographic-principle Sep 25, 2019

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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

src/namespace.tests.ts Outdated Show resolved Hide resolved
@tlaundal tlaundal merged commit 5232135 into master Sep 27, 2019
@tlaundal tlaundal deleted the qualifiers branch October 8, 2019 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants