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(common): introduce KeyValuePipe #24319

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Toxicable
Contributor

Toxicable commented Jun 6, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

There is no way to iterate a object or Map

Closes: #24200
Continuation from: #11319

What is the new behavior?

A pipe to object Map or object (dictionary) to a key value pair array for use in a ngFor

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@googlebot googlebot added the cla: yes label Jun 6, 2018

@Toxicable Toxicable referenced this pull request Jun 6, 2018

Closed

feat(common): new pipe to support object for ngFor #11319

3 of 3 tasks complete
value: V;
}
@Pipe({name: 'mutableToKeyValue', pure: false})

This comment has been minimized.

@mhevery

mhevery Jun 6, 2018

Member

I don' think the word mutable should be there. It is inconsistent with the rest of the systems naming.

This comment has been minimized.

@Toxicable

Toxicable Jun 6, 2018

Contributor

Changed to keyvalue similar to the lowercase on LowerCasePipe

this.differ = this.differs.find(input).create();
}
// TODO: shouldnt the differ allow Map?

This comment has been minimized.

This comment has been minimized.

@Toxicable

Toxicable Jun 6, 2018

Contributor

I get a type error if I pass input directly

This comment has been minimized.

@Toxicable

Toxicable Jun 6, 2018

Contributor

specifically:

Argument of type '{ [key: string]: V; [key: number]: V; } | Map<K, V>' is not assignable to parameter of type '{ [key: string]: V; }'.
  Type 'Map<K, V>' is not assignable to type '{ [key: string]: V; }'.
    Index signature is missing in type 'Map<K, V>'
let nextValue: Array<KeyValuePair<string, V>>;
// always return a new array ref at this
if (input instanceof Map) {
// keys maintain their type here

This comment has been minimized.

@mhevery

mhevery Jun 6, 2018

Member

I don't think we should be stringifing it here. What is your reasoning for it?

This comment has been minimized.

@Toxicable

Toxicable Jun 6, 2018

Contributor

Object.keys converts all keys to strings. So to be consistent we should do the same with Map

.map(key => this.makeKeyValuePair(String(key), input.get(key)));
} else {
// all keys will be converted into strings when using Object.keys()
nextValue = Object.keys(input).sort().map(key => this.makeKeyValuePair(key, input[key]));

This comment has been minimized.

@mhevery

mhevery Jun 6, 2018

Member

map is not very performant. Given that this will be in the hotpath could we implement it with a for loop instead?

This comment has been minimized.

@Toxicable

Toxicable Jun 6, 2018

Contributor

Done.
Still not overly happy with Array.from(input.keys()) since it's an extra iteration but I don't see another way to sort it without resolving the iterator into an Array

import {KeyValueDiffer, KeyValueDiffers, Pipe, PipeTransform} from '@angular/core';
export interface KeyValuePair<K, V> {

This comment has been minimized.

@mhevery

mhevery Jun 6, 2018

Member

I think KeyValue is shorter. I don't think the word Pair adds value.

transform(input: null | {
[key: string]: V;
[key: number]: V;
} | Map<K, V>): Array<KeyValuePair<string, V>> | null;

This comment has been minimized.

@mhevery

mhevery Jun 6, 2018

Member

I would expect that KeyValuePair should be exported as well since it would be part of the public API, but I don't see it in this file.

@@ -14,6 +14,7 @@
import {AsyncPipe} from './async_pipe';
import {LowerCasePipe, TitleCasePipe, UpperCasePipe} from './case_conversion_pipes';
import {DatePipe} from './date_pipe';
import {MutableToKeyValuePipe} from './entries_pipe';

This comment has been minimized.

@mhevery

mhevery Jun 6, 2018

Member

I would expect KeyValuePair to be exported as well since it is part of public API.

@vicb vicb added the comp: common label Jun 6, 2018

@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Jun 7, 2018

@mhevery I've addressed your feedback

@Toxicable Toxicable changed the title from Entries pipe to feat(common): introduce KeyValuePipe Jun 7, 2018

@mhevery

I am sorry I missed the docs the first time through. Hopefully this is the last pass.

}
@Pipe({name: 'keyvalue', pure: false})
export class KeyValuePipe<K, V> implements PipeTransform {

This comment has been minimized.

@mhevery

mhevery Jun 7, 2018

Member

Sorry I did not notice this earlier. Because this is public API it needs to have documentation. See example here: https://github.com/angular/angular/blob/master/packages/common/src/pipes/date_pipe.ts#L14-L105

This comment has been minimized.

@Toxicable

Toxicable Jun 7, 2018

Contributor

Never been very good at docs, let me know if that'll suffice

return nextValue;
}
private makeKeyValuePair(key: number|string, value: any): KeyValue<string, V> {

This comment has been minimized.

@mhevery

mhevery Jun 7, 2018

Member

This method would be better as free floating function since it would minify better. (It does not use this hence it does not need to be a member of the class)

import {KeyValueDiffer, KeyValueDiffers, Pipe, PipeTransform} from '@angular/core';
export interface KeyValue<K, V> {
key: K;

This comment has been minimized.

@mhevery

mhevery Jun 7, 2018

Member

since K is always a string there is no need for K

let nextValue: Array<KeyValue<string, V>> = [];
if (input instanceof Map) {
const keys = Array.from(input.keys()).sort();

This comment has been minimized.

@mhevery

mhevery Jun 7, 2018

Member

If keys are not strings than the sort will fail. I think you need to convert to string before sort()

This comment has been minimized.

@Toxicable

Toxicable Jun 7, 2018

Contributor

Unless i've misinterpted you, sort can handle number just fine.
take this example
image
The reason im hesitant to simply convert it is that it'll be a whole extra iteration.
Also I do have a test for this https://github.com/angular/angular/pull/24319/files#diff-b6bf05a02506b3cd96c753d3a8d0d30aR72
maybe I should add one with alpha and numerical

This comment has been minimized.

@Toxicable

Toxicable Jun 7, 2018

Contributor

I think I get what you mean: when the input is a function, object etc.
I think adding a param for a sort comparitor function would be the best idea here. With the default behaviour being .sort() so leave it to the dev to add the comparitor if they have complex keys

for (let i = 0; i < keys.length; i++) {
const key = keys[i];
// input could possibly be Megamorphic here
nextValue.push(this.makeKeyValuePair(String(key), input[key]));

This comment has been minimized.

@mhevery

mhevery Jun 7, 2018

Member

No need to convert to string since Object.keys will always return strings

This comment has been minimized.

@Toxicable

Toxicable Jun 7, 2018

Contributor

Thanks, missed that in refactoring

@alfaproject

This comment has been minimized.

Contributor

alfaproject commented Jun 7, 2018

A question before this becomes public API: shouldn't the sort be optional? (maybe on by default, but configurable at least?)

@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Jun 8, 2018

@mhevery I've added a compareFn to deal with the sorting issues but i've become a bit uncertain about the implementation now.
There are many any casts since the types don't match up between Map and Object since the compareFn is really only for Map (since you cant have complex types as the key in a Object)

* The output array will be ordered by keys.
* By default the comparator will be by Unicode point value
* You can optionally pass a compareFn if your keys are complex types.
*

This comment has been minimized.

@mhevery

mhevery Jun 11, 2018

Member

Could you add an example of usage. We are trying to make sure that all new APIs have an example. See as an example of how to do it https://github.com/angular/angular/blob/master/packages/common/src/pipes/async_pipe.ts#L43-L68. Notice that the example https://github.com/angular/angular/blob/master/packages/examples/common/pipes/ts/async_pipe.ts has tests https://github.com/angular/angular/blob/master/packages/examples/common/pipes/ts/e2e_test/pipe_spec.ts which verifies that the example does not get broken.

this.differ = this.differs.find(input).create();
}
// TODO: shouldnt the differ signature allow Map?

This comment has been minimized.

@mhevery

mhevery Jun 11, 2018

Member

shouldnt => shouldn't

The issue is that you declared your type as {} | Map where as KeyValueDiffer has: diff(object: Map<K, V>): KeyValueChanges<K, V>; and diff(object: {[key: string]: V}): KeyValueChanges<string, V>; overloaded methods. The issue is that neither {} or Map is a subtype of {}|Map. To make this work you would have to declare diff(object: {[key: string]: V}|Map<K,V> ): KeyValueChanges<K|string, V>; or something like that, but than you would love the type information on string.

So casting to any is a reasonable compromise.

Alternatively you could do:

    const differChanges = input instanceof Map ? this.differ.diff(input) : this.differ.diff(input);

Which types but is very weird.

I would remove the comment/TODO.

return {key: key as any, value};
}
export interface KeyValue<V> {

This comment has been minimized.

@mhevery

mhevery Jun 11, 2018

Member

KeyValue is part of public API, it needs docs. (example not needed)

@mhevery

This comment has been minimized.

Member

mhevery commented Jun 11, 2018

OK, I thought about it some more and I have done some refactoring to clean up the code and the type system. I pushed the changes here 97fbe77

@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Jun 12, 2018

@mhevery Thanks man, that helps a lot, will make those final few docs and tests tonight

*
*/
@Pipe({name: 'keyvalue', pure: false})
export class KeyValuePipe implements PipeTransform {

This comment has been minimized.

@alfaproject

alfaproject Jun 13, 2018

Contributor

Any reason not to do KeyValuePipe<K, V>? It down-levels to the exact same code and would avoid having to use any, I think.

@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Jun 13, 2018

@mhevery Addressed all your feedback and added more tests / docs.

The sourcelab failure looks unrelated to my changes

@mhevery

This comment has been minimized.

Member

mhevery commented Jun 13, 2018

@Toxicable Thank you for all of your patience in getting this into mergeable state. 👍

@mhevery

This comment has been minimized.

@mhevery mhevery closed this in 92b278c Jun 13, 2018

mhevery added a commit that referenced this pull request Jun 13, 2018

mhevery added a commit that referenced this pull request Jun 13, 2018

@Toxicable Toxicable deleted the Toxicable:entries-pipe branch Jun 13, 2018

@mhevery mhevery referenced this pull request Sep 7, 2018

Closed

fix(core): correct the nullability of kvDiffer #21623

1 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment