fable-compiler / Fable

F# to JavaScript, TypeScript, Python, Rust and Dart Compiler
http://fable.io/
MIT License
2.92k stars 300 forks source link

Bring back [<POJO>]? #2421

Closed absolutejam closed 3 years ago

absolutejam commented 3 years ago

Description

In numerous occasions, I find myself having to use anonymous records for a POJO and this discards a whole bunch of type-safety and conciseness (destructuring & reusability) when I'm able to re-use a defined type. I noticed in older examples there's a [<POJO>] attribute which is exactly what I'm in need of, and I've also seen a couple of other people mention this in the Slack too.

This would obviously open up destructuring, but also allow a record as React component props (I think?). Additionally, it makes interop a lot easier as it doesn't mean dropping down to potentially unsafe anonymous records.

Repro code

[<POJO>]
type Foo =
    {
        A: string
        B: int
    }

would become something like

{ A: "whatever", B: 10 }

Was this removed for a technical reason or was it just to remove the amount of options?

Would you be open to this returning?

Zaid-Ajaj commented 3 years ago

but also allow a record as React component props (I think?)

A record can already be used as input props for React components, except that due to limitations of react-refresh (the hot module reloading plugin) the record type must be defined in a separate module

module PropTypes 

type Foo = { A: string; B: int }

It work fine if you define the record type in the same module but during development when you edit your component, it will reset the state instead of just rerendering

Was this removed for a technical reason

I think it was removed because it behaved differently than normal records in structural equality and reflection.

Would you be open to this returning?

@alfonsogarciacaro would it be possible to handle [<Pojo>] records the exact same way anonymous records are handled? (emit reflection metadata like usual, handle structural equality)

alfonsogarciacaro commented 3 years ago

@ncave is working on #2279 to extend the "erase declaration" feature (currently only available for some unions with the Erase attribute) so you can use it to remove the declarations of all records and unions while keeping most features like reflection and structural equality. That would have similar effect to the old Pojo attribute. Attached members (currently used for interfaces and overrides) won't be possible however because these require a prototype.

We're just in doubt if the feature:

  1. can be applied to all union and records through a compiler flag
  2. can be applied to individual unions and records with the Erase attribute.

In the case of 1. we should probably add a NoErase attribute to opt-out the feature, but the main issue is with libraries: will the records/unions be erased also in this case, or do we need to add metadata to the libraries so they can set their own compiler options (iirc we had a similar discussion in the past and we decided not to do it).

ncave commented 3 years ago

@alfonsogarciacaro Technically #2279 is to erase to named tuples (arrays), not POJOs. But your point about attached members is a valid one, I'm still unsure how to proceed about that. For both tuple arrays and POJOs, can we attach members to the object itself, not to the prototype? Or is that going to bloat it too much? We don't have that many attached members anymore, so it may be ok, what do you think?

alfonsogarciacaro commented 3 years ago

Ah, ok! I thought records were being erased to POJOs 😅 This adds another variable to the equation, some people may want to erase records as POJOs for better compatibility and other as arrays for maximum bundle size efficiency 🤔 Hmm, maybe this complicates things but probably the safest way for now is to introduce the feature in a per-case basis with the Erase attribute and add an optional argument to the attribute to specify if records should be erased as arrays or pojos (likely the default).

In any case, as discussed in the PR, I'd like to remove ErasedType from the Fable AST and add it instead as an info property to entities so we can do the erasing later in the Fable2Babel step to reduce complexity in the AST and avoid breaking the plugins. I will try to do that.

ncave commented 3 years ago

@alfonsogarciacaro It's already removed, there are no AST modifications in the #2279, the erasing has been moved to Fable2Babel. It's still unfinished and WIP though.

alfonsogarciacaro commented 3 years ago

Ups, I should have checked the PR better 😅 I've been having a look but it seems my hope of just extending the behavior of Erase attribute won't be possible because it will conflict with the current uses. I will try to list all the possibilities later for discussion.

alfonsogarciacaro commented 3 years ago

Sorry for the delay! I'll try to summarize the current situation and the alternatives to add more possibilities to erase class declarations in the generated JS for unions/records.

Currently

Proposal

Allow removing the declaration of more records/unions by either a compiler flag or specialized attributes. This can have the following pros:

But there will be also the following cons:

How to erase unions

If we want to keep access to the case name, we should likely always erase to a JS array where the first item is the case name. Or if we make the first item be the tag we can use the reflection info to access the case name when needed. In any case, the main drawback of always erasing to an array is it would conflict with the current behavior of Erase so we would have to either add an alternative attribute or use another method to decide which unions would be erased.

How to erase records

Two main options:

  1. As a pojo: probably what the user expects as it's how anonymous records work now.
  2. As an array: this may be better for bundle size as we don't need the field names but it will likely cause issues with interop and debugging, and have worse backwards-compatibility.

Should we decide on one of the two ways? Or give the user the ability to choose either?

alfonsogarciacaro commented 3 years ago

Closing for now, as we're not currently working on changing the way records and unions are represented in JS. If necessary we can continue discussion in the experimental PR open by @ncave #2279