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

feat(store): implement iif operator #778

Merged
merged 8 commits into from
Jan 18, 2019
Merged

feat(store): implement iif operator #778

merged 8 commits into from
Jan 18, 2019

Conversation

arturovt
Copy link
Member

PR Checklist

PR Type

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #545

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

packages/store/operators/src/iif.ts Outdated Show resolved Hide resolved
@arturovt
Copy link
Member Author

@eranshmil @markwhitfeld 🚀

@arturovt
Copy link
Member Author

arturovt commented Jan 17, 2019

codeclimate whines about missing semicolon, if i add this semicolon - prettier anyway removes it :D

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

Great work! Love the comprehensive tests.
Just some suggested changes, test name tweaks, and also to keep the code around each operator in the same file.

const operatorAndExistingNotProvided = !isStateOperator(operatorOrValue) && !existing;
if (operatorAndExistingNotProvided) {
return operatorOrValue as T;
}
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that you could leave out these 4 lines of code above and it would all still work perfectly ;-)

return function iifOperator(existing: Readonly<T>): T {
// If condition is not `null` and not `undefined`
let result = !!condition;
// If condition is a function
Copy link
Member

Choose a reason for hiding this comment

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

// Convert the value to a boolean
let result = !!condition;
// but if it is a function then run it to get the result


export type PatchValues<T> = {
readonly [P in keyof T]?: T[P] extends (...args: any[]) => infer R ? R : T[P]
};
Copy link
Member

Choose a reason for hiding this comment

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

I see the idea with moving these into a shared file but I can't see any case where they would be used by other operators so I think that they should move back to their respective operator files. Just keep the operators code together nicely.

return typeof value === 'undefined';
}

export function isPredicate<T>(value: Predicate<T> | boolean): value is Predicate<T> {
Copy link
Member

Choose a reason for hiding this comment

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

This could move back into the iif.ts file for now, nothing else uses it.

export function isStateOperator<T>(value: T | StateOperator<T>): value is StateOperator<T> {
return typeof value === 'function';
}

export function isUndefined(value: unknown): value is undefined {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this function provided by typescript in its' standard lib?

Copy link
Member Author

Choose a reason for hiding this comment

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

@markwhitfeld unfortunately :(

});
});

describe('when undefined provided', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe('when undefined provided', () => {
describe('when undefined condition provided', () => {

import { patch } from '../src/patch';

describe('iif', () => {
describe('when null provided', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe('when null provided', () => {
describe('when null condition provided', () => {

expect(newValue).toBe(original);
});

it('returns the same root if "else" condition is provided', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('returns the same root if "else" condition is provided', () => {
it('returns the same root if "else" provides same value as existing', () => {

expect(newValue).toBe(original);
});

it('returns patched object if "else" condition is provided', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('returns patched object if "else" condition is provided', () => {
it('returns patched object if "else" is provided with a new value', () => {

expect(newValue).toBe(original);
});

it('returns the same root if "else" value provided', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('returns the same root if "else" value provided', () => {
it('returns the same root if "else" provides same value as existing', () => {

@arturovt
Copy link
Member Author

arturovt commented Jan 18, 2019

@markwhitfeld ping 🚀

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

Awesome work

@markwhitfeld markwhitfeld merged commit 55c7761 into ngxs:master Jan 18, 2019
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.

4 participants