Separate operation API from implementation

[couzic]

I started implementing the key alternative to the selector for the associateBy() operation, but in the process I realized the operation would be better defined as an overloaded method in the Sequence API, with a single function acting as the implementation (much like what has been done for the ComparatorFactory).

I think I ended up with a pretty decent solution, currently it's just a POC where only the associateBy() operation has been modified. I pushed it here:
https://github.com/couzic/sequency/tree/operations-as-methods

I didn't submit a pull request because of the impact it has on the docs (no breaking changes in the API though). Currently the operations are documented in their respective modules. With the new solution, the Sequence class displays its operators as methods, with all the overloaded variations.

@winterbe it would be nice if you could take a look and compare with the current solution. The two major benefits I see are:

• Improved type-safety by defining separate overloaded method signatures that are non-ambiguous (in this case, associateBy() now either returns a Map<K, T> or a Map<K, V>, where it used to return a Map<K, T | V>)
• More intuitive documentation, all operations are now directly visible as Sequence methods

Anyway, if you agree with the overall direction this is taking, I'll migrate all the operators until the documentation is consistent, then submit a pull request.

[winterbe]

Thanks for your effort Mikael. I'll look into it soon. Mikael Couzic <notifications@github.com> schrieb am Do. 5. Okt. 2017 um 12:34:

…

I started implementing the key alternative to the selector for the associateBy() operation, but in the process I realized the operation would be better defined as an overloaded method in the Sequence API, with a single function acting as the implementation (much like what has been done for the ComparatorFactory). I think I ended up with a pretty decent solution, currently it's just a POC where only the associateBy() operation has been modified. I pushed it here: https://github.com/couzic/sequency/tree/operations-as-methods I didn't submit a pull request because of the impact it has on the docs (no breaking changes in the API though). Currently the operations are documented in their respective modules. With the new solution, the Sequence class displays its operators as methods, with all the overloaded variations. @winterbe <https://github.com/winterbe> it would be nice if you could take a look and compare with the current solution. The two major benefits I see are: - Improved type-safety by defining separate overloaded method signatures that are non-ambiguous (in this case, associateBy() now either returns a Map<K, T> or a Map<K, V>, where it used to return a Map<K, T | V>) - More intuitive documentation, all operations are now directly visible as Sequence methods Anyway, if you agree with the overall direction this is taking, I'll migrate all the operators until the documentation is consistent, then submit a pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdmqWevsmdIPyJ6tqVuR7zZ9Uo9GZs-ks5spLCygaJpZM4Pu3yp> .

[winterbe]

@couzic I've looked into your work and generally like it. Both benefits mentioned are definitely worth the effort it would take to refactor all operations.

I'd like to discuss a few things with you:

I'm little confused about using the name Sequence both for the interface and the class. I wasn't aware this is even possible in TypeScript since it's prohibited in other languages such as Java or Kotlin.

I'm also wondering if operations could be better implemented via mixins instead of interfaces + functions: https://www.typescriptlang.org/docs/handbook/mixins.html

Mixins could possibly reduce the boilerplate code of assigning prototype methods on Sequence manually. Also imports would be more simple (single import per operation). But I'm not entirely sure how docs would look like with using mixins. Hopefully typedoc works well with mixins. I haven't used mixins before though.

What do you think about using mixins?

[couzic]

Yes, TypeScript is quite flexible, if you declare a type twice it will merge the declarations. But after thinking about it, it might be better to export only the interface and hide the implementation class.

I think the mixin solution should work with typedoc, since our Sequence type explicitely implements the operator interface. I'll give it a try.

Also, I haven't tested it yet but I believe we have the opportunity to completely hide the operator modules from the docs. The idea would be to only have the operators documented on the Sequence type, hence removing duplication in the docs.

[winterbe]

That would be great. The docs are quite verbose currently. Mikael Couzic <notifications@github.com> schrieb am Fr. 6. Okt. 2017 um 17:42:

…

Yes, TypeScript is quite flexible, if you declare a type twice it will merge the declarations. But after thinking about it, it might be better to export only the interface and hide the implementation class. I think the mixin solution should work with typedoc, since our Sequence type explicitely implements the operator interface. I'll give it a try. Also, I haven't tested it yet but I believe we have the opportunity to completely hide the operator modules from the docs. The idea would be to only have the operators documented on the Sequence type, hence removing duplication in the docs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdmqZsaAMzwaVp4R5eDaXIz-bjeq7Irks5spkpggaJpZM4Pu3yp> .

[couzic]

@winterbe I pushed a change where I partly use the mixin approach described in the docs, now only one import is required. Also, the implementation detail of the operation is hidden from the docs. However we still have two (minor) problems in the docs I'd like to address:

• The Sequence type in the docs appears as Sequence<T, T>. probably due to the double declaration of Sequence both as a class and an interface.

• The docs are still verbose, the operations being described both in the Sequence type and in their respective modules.

I have a few ideas in mind, I'll try them when I get the time later this week.

[winterbe]

Interesting. Maybe the these problems could be solved by getting rid of the interfaces (AssociateBy, Sequence) at all and just use classes? Don't get me wrong, interfaces are great but in this case only one implementation exists, so there's no real need to use interfaces over plain classes in my opinion.

From reading the docs about mixins this should be possible class Sequence<T> implements AssociateBy<T> where AssociateBy is not an interface but a class.

What do you think about it? I'm looking forward to your ideas.

[couzic]

I merged the class and interface declarations in AssoicateBy, you're right the interface was not necessary in this case.

We can do the class Sequence<T> implements AssociateBy<T> thing but that forces us to provide dummy implementations of each operator in the Sequence class, something like:

export default class Sequence<T> implements AssociateBy<T> {
    associateBy = () => null as any;
}

... and in fact we can't, because I just tested and typedoc will document the dummy implementation only.

[couzic]

One of my ideas is to completely hide the implementation class, and only export the interface, something like:

/**
 * A Sequence accepts an iterator and provides a fluent functional API consisting
 * of various intermediate and terminal operations for processing the iterated data.
 * The operations are evaluated lazily to avoid examining all of the input data
 * when it's not necessary. Sequences can be iterated only once.
 */
export interface Sequence<T> extends AssociateBy<T>, ... {
}

class SequenceImpl<T> { // NO EXPORT
}
applyMixins(SequenceImpl, [AssociateBy, ...]);

That way typedoc will have an easy job, and we are free to mess up with the implementation class behing the scenes.

[couzic]

I also created an issue for TypeDoc:
TypeStrong/typedoc#618

Maybe someone will pick it up

[winterbe]

Exporting Sequence as an interface while hiding the actual implementation is a good idea. However we have to make sure, that this concept works for all operations. E.g. take a look at average which is restricted to this: Sequence<number>. I guess it shouldn't be a problem but we have to test this explicitly before deciding for fundamental structural changes.

Does your example above work with mixin classes, so AssociateBy could be a class instead of interface?

[winterbe]

Another interesting aspect which come to my mind is user-defined operations. Take a look at how sequences are implemented in Kotlin:

https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.sequences/-sequence/index.html

Sequence is an interface with a single method iterator(). All operations are implemented as extension functions upon this interface. This makes writing new user-defined sequence operations trivial.

We should evaluate how extensions could be achieved in Sequency. In JavaScript this would be trivial by just adding your function to the Sequence prototype. But what about TypeScript?

How would hiding the implementation influence user-defined extensions?

[couzic]

Here's what I got:
https://github.com/couzic/sequency/tree/hide-sequence-implementation

• All operations are exported as classes. The Sequence interface extends all these classes, so we achieve the mixin at the type level.

• The hidden SequenceImpl class is augmented with the same classes, so we achieve the mixin at the implementation level.

• I had to provide a couple functions in the Sequence module so all operators would still work without direct access to the now hidden implementation class:

  • createSequence() wich accepts a SequenceIterator
  • isSequence() to use as a substitute to instanceof Sequence

• Operator aliases are directly defined in the original operator class. For example:

export class FirstOrNull {

    /**
     * Returns the first element of the sequence or the first element matching `predicate` if present, otherwise returns `null`.
     *
     * @param {(T) => boolean} predicate
     * @returns {T}
     */
    firstOrNull<T>(this: Sequence<T>, predicate?: (T) => boolean): T | null {
        if (predicate != null) {
            return this.filter(predicate).firstOrNull();
        }
        return this.iterator.hasNext()
            ? this.iterator.next()
            : null;
    }

    /**
     * Returns the first element of the sequence or the first element matching `predicate` if present, otherwise returns `null`.
     *
     * @param {(T) => boolean} predicate
     * @returns {T}
     */
    find<T>(this: Sequence<T>, predicate?: (T) => boolean): T | null {
        return this.firstOrNull(predicate);
    }

}

• Documentation of the Sequence type has improved. All operations are listed as methods, so it's now possible to overload them.

• User-defined operations are now supported.

  • On the type level, the user should augment the Sequence type with:

declare module "sequency" {
    export default interface Sequence<T> {
        userDefinedOperation(this: Sequence<T>): T;
    }
}

• On the implementation level, the user can use one of the following methods:
  • augmentSequenceWithOperator() which accepts a function
  • augmentSequenceWithMixin() which accepts a class

There are things, especially that last part (user defined operations), that may need some refinement. Maybe we should only support the Class/Mixin approach. But in general, I see only benefits from the change. What do you think ?

[winterbe]

Wow, I’m super excited about your work, thanks. I’m pretty satisfied with the overall code structure and looking forward to see the new docs.

Please open a pull request so I can finally review and merge the changes. 👍

[couzic]

Cool !
Here you go !

[winterbe]

I've merged #11. Thanks again for this huge contribution. I really appreciate all the effort you've put into this. I'll publish a new version to npm soon.