Add concatenation #15
Conversation
Co-Authored-By: Abbondanzo <peter@abbondanzo.com>
|
@colbyr If you get a chance, I'd love to hear your thoughts on this. We'll be looking to merge this sometime next week. |
There was a problem hiding this comment.
Ah cool this'll be useful. I left some comments inline about modifying the protocol definition.
Also I was wondering about the intended behavior. (I don't really have strong opinions I just want to make sure we're clear on what it does/doesnt do 😄)
Does concat only work with two similar types (e.g. Arrray+Array, Map+Map) or can you mix and match? e.g. concat(Map({ a: 'a' }), List.of('b')). If it's meant to work with mixed types it might be good to explore that in the tests+docs.
Also when you use it with two Maps or Objects is it basically the same as a merge? If so do you think it's worth having that same behavior in two different functions? If we wanted, we could limit concat to only work on indexed types like List, Array, Seq, etc.
src/internal/TransmuteCollection.js
Outdated
| * @param {TYPE<_, V>} subject | ||
| * @return {Seq.Indexed<V>} | ||
| */ | ||
| export const indexedSeq = protocol({ |
There was a problem hiding this comment.
What's this new indexedSeq protocol for? It doesn't seem like it's used anywhere.
There was a problem hiding this comment.
The original implementation used to build sequences based on whether or not the subject was an indexed or a keyed iterable, and reconstruct the object using that new sequence. I figured since the work was already done, there's no reason to toss it out.
There was a problem hiding this comment.
The idea of TransmuteCollection was to have a set of protocols that are used internally by the library, so if you have a custom type in client code you can implement the protocols and that type would work with transmute. I'd lean away from adding an indexedSeq protocol until it's used by something in the library.
|
@colbyr However, I am unable to target |
There was a problem hiding this comment.
However, I am unable to target Iterable.Indexed in the implementInherited method, limited to only target Iterable. Else, the protocol is unable to pick up on immutable objects like Seq and List.
Hmm could you target Collection.Indexed and Seq.Indexed instead? That seems to get us the dispatching you want:
src/internal/TransmuteCollection.js
Outdated
| * @param {TYPE<_, V>} subject | ||
| * @return {Seq.Indexed<V>} | ||
| */ | ||
| export const indexedSeq = protocol({ |
There was a problem hiding this comment.
The idea of TransmuteCollection was to have a set of protocols that are used internally by the library, so if you have a custom type in client code you can implement the protocols and that type would work with transmute. I'd lean away from adding an indexedSeq protocol until it's used by something in the library.
|
Targeting |
|
^ force pushed over the history to remove the |
src/__tests__/concat-test.js
Outdated
|
|
||
| it('uses the subject Seq as the return value type', () => { | ||
| // https://github.com/facebook/jest/issues/5998 | ||
| expect(concat(['test'], Seq('test')).toJS()).toEqual([ |
There was a problem hiding this comment.
These tests don't validate the type since you're doing the toJS. I'd find another way to validate the return type or remove the test (but I think having a test for this would be useful).
There was a problem hiding this comment.
I linked the jest issue in question but this is happening due to reasons out of our control. The best we can do is check that the contents match and that the returned object is an instance of Seq.
Co-Authored-By: Kevin Johnson <kevin@coolkev.com>
Co-Authored-By: Kevin Johnson <kevin@coolkev.com>
Co-Authored-By: Kevin Johnson <kevin@coolkev.com>
Adds
concatoperator which merges indexed iterables and joins keyed iterables.