ListCollector

[nojaf]

An initial naive implementation for #3700.

I'm a little confused that FSharp.Core.CompilerServices.js looks fine:

import { addRangeInPlace } from "./Array.js";
import { toList } from "./Seq.js";
import { class_type } from "./Reflection.js";
export class ListCollector$1 {
    constructor() {
        this.collector = [];
    }
    Add(value) {
        const this$ = this;
        void (this$.collector.push(value));
    }
    AddMany(values) {
        const this$ = this;
        addRangeInPlace(values, this$.collector);
    }
    AddManyAndClose(values) {
        const this$ = this;
        addRangeInPlace(values, this$.collector);
        return toList(this$.collector);
    }
    Close() {
        const this$ = this;
        return toList(this$.collector);
    }
}
export function ListCollector$1_$reflection(gen0) {
    return class_type("Microsoft.FSharp.Core.CompilerServices.ListCollector`1", [gen0], ListCollector$1);
}
export function ListCollector$1_$ctor() {
    return new ListCollector$1();
}

but the usage in QuickTest.js does not:

import { some } from "./fable_modules/fable-library.4.11.0/Option.js";
import { defaultOf, equals } from "./fable_modules/fable-library.4.11.0/Util.js";
import { startsWith, isNullOrEmpty, toFail, printf, toConsole } from "./fable_modules/fable-library.4.11.0/String.js";
import { startImmediate } from "./fable_modules/fable-library.4.11.0/Async.js";
import { singleton } from "./fable_modules/fable-library.4.11.0/AsyncBuilder.js";
import { ListCollector$1__Close, ListCollector$1__Add_2B595 } from "./fable_modules/fable-library.4.11.0/FSharp.Core.CompilerServices.js";

// ...

testCase("meh2", () => {
    let copyOfStruct_1;
    const c = defaultOf();
    let copyOfStruct = c;
    ListCollector$1__Add_2B595(copyOfStruct, 1);
    (copyOfStruct_1 = c, ListCollector$1__Close(copyOfStruct_1, void 0));
});

Am I missing something obvious @MangelMaxime @ncave?

[MangelMaxime]

This is probably because you used [<AttachMembers>] on the ListCollector type definition.

This tells Fable attach members to the JavaScript class instead of generated a function outside of the type like ListCollector$1__Add_2B595.

With [<AttachMembers>]

import { class_type } from "fable-library/Reflection.js";

export class ListCollector$1 {
    constructor() {
        this.collector = [];
    }
    Add(value) {
        const this$ = this;
        void (this$.collector.push(value));
    }
}

export function ListCollector$1_$reflection(gen0) {
    return class_type("Test.ListCollector`1", [gen0], ListCollector$1);
}

export function ListCollector$1_$ctor() {
    return new ListCollector$1();
}

Without [<AttachMembers>]

import { class_type } from "fable-library/Reflection.js";

export class ListCollector$1 {
    constructor() {
        this.collector = [];
    }
}

export function ListCollector$1_$reflection(gen0) {
    return class_type("Test.ListCollector`1", [gen0], ListCollector$1);
}

export function ListCollector$1_$ctor() {
    return new ListCollector$1();
}

export function ListCollector$1__Add_2B595(this$, value) {
    void (this$.collector.push(value));
}

If you remove [<AttachMembers>] it will probably works.

In general [<AttachMembers>] is only use for interop with JavaScript so I don't think we are using it in fable-library.

The alternative would be to specifically rewrite how Fable generates the code for this type/members.

For info using [<AttachMembers>] comes with limitations, as for example it does not support method overload because JavaScript method doesn't support it.

[nojaf]

Removing those attributes doesn't help that much.
I believe something I'm not taking into account is that ListCollector is a struct in F# Core.

[ncave]

@nojaf See PR#1

[nojaf]

Thanks again @ncave for fixing this!
@MangelMaxime ready for review.

[MangelMaxime]

Can we add tests for each method available and also edges cases like calling .close() on an empty ListCollector?

[nojaf]

Let's not overextend. I'm including only the parts that interest me, as this is largely experimental. The code is already solid—something we're both aware of—and this pull request improves its current state.

[MangelMaxime]

I don't agree yes for this module it is probably enough, but it happens several times that doing so allowed us to catch behaviour we were not aware of.

There are still some APIs in fable-library that don't behave correctly because they didn't have a test associated for every usage. Having tests also allows us to catch on regressions.

I added the tests to cover the most common cases.

[MangelMaxime]

Looking at the code it seems like we can remove the mutable here

[nojaf]

That would not reflect the actual usage of shared code. In regular F# the collector needs to be mutable.

[MangelMaxime]

Didn't know about that.

[MangelMaxime]

Thanks for the implementation, I will include it in the next release which should come out this weekend.

[nojaf]

Thanks for going the extra mile here!