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

Add flag to disable reflection #2710

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Add flag to disable reflection #2710

merged 1 commit into from
Jan 11, 2022

Conversation

alfonsogarciacaro
Copy link
Member

@kerams
Copy link
Contributor

kerams commented Apr 23, 2022

@alfonsogarciacaro, would it be possible to also have an attribute that we could selectively apply on types? Sort of use at your own risk thing? Very often you do need reflection in a part of the application (serializing data contract types in Remoting for instance) but not in another (Elmish models, messages).

For

type Msg =
    | TabClicked
    | UpdatePasswordClicked
    | UpdateProfileClicked
    | UpdateAccountClicked

I get

import { Union } from "fable-library/Types.js";
import { union_type } from "fable-library/Reflection.js";

export class Msg extends Union {
    constructor(tag, ...fields) {
        super();
        this.tag = (tag | 0);
        this.fields = fields;
    }
    cases() {
        return ["TabClicked", "UpdatePasswordClicked", "UpdateProfileClicked", "UpdateAccountClicked"];
    }
}

export function Msg$reflection() {
    return union_type("Test.Msg", [], Msg, () => [[], [], [], []]);
}

and none of that is treeshaken by Terser. What I would like is [<Fable.Core.NoReflection>] that would produce:

import { Union } from "fable-library/Types.js";

export class Msg extends Union {
    constructor(tag, ...fields) {
        super();
        this.tag = (tag | 0);
        this.fields = fields;
    }
}

(When no case has any fields, perhaps fields could be removed from the ctor? But that's a whole different request.)

@kerams
Copy link
Contributor

kerams commented Apr 23, 2022

Actually, looking at that desired output, there's no need to emit anything since you could just use the basic Union for all unions with no reflection as there's nothing specific to the original F# DU anymore. The size savings would thus be even larger.

@MangelMaxime
Copy link
Member

I add completely missed that you added this flag @alfonsogarciacaro. 😅

@kerams I just want to point that in case you are using Elmish.Debugger you are going to need the reflection information because it is using Thoth.Json to transform into JSON for passing the information to Redux dev tools.

@kerams
Copy link
Contributor

kerams commented Apr 24, 2022

@MangelMaxime, sure but in that case you can conditionally compile the attribute just for production :).

@MangelMaxime
Copy link
Member

@kerams Yes sure, just wanted to point it out in case you encounter this bug in the future.

Because, I don't know what error / warning would be emitted by Fable in such a case and if it would be user friendly or not.

@kerams
Copy link
Contributor

kerams commented Apr 24, 2022

If you use it in your code and something breaks, it's your problem (Don't touch the attribute unless you know what you're doing, much like Erase). It might be more of an issue when it's baked in a third party library.

@alfonsogarciacaro
Copy link
Member Author

Thanks @kerams. I think it makes sense to have an attribute to regulate this behavior on a per-type basis. Although we need to be careful because adding more specific behavior increases complexity and sometimes it doesn't actually bring the benefits we expect. Some notes:

  • I added this flag because it was a minor thing and in principle it was intended for JS libraries as @MangelMaxime originally asks. I'm not very concerned about the effect of reflection on the bundle size because bundlers should be able to remove the $reflection methods if they're not used.
  • Your proposal focuses on unions and goes beyond discarding the $reflection method, it also discards separated classes for each union. This has many effects on type testing, printing, equality, the ability to implement interfaces, etc. This is tricky because it can be abused quickly, and I know from experience users always want the benefits but none of the drawbacks 😉
  • Because of this, your proposal reminds me more of the previous work by @ncave in [WIP] Erase unions and records #2279 to optimize records and unions by reducing them to their minimal shape. Maybe we could revisit that PR (adapting it so it can be activate per-type with an attribute instead of/as well as with a compiler switch) and give users the option to optimize union/records that are only used as light data-transports.

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.

3 participants