Skip to content

[WIP] Compatibility with new Fable.React#34

Closed
alfonsogarciacaro wants to merge 6 commits into
masterfrom
compatibility-with-latest-fable-react
Closed

[WIP] Compatibility with new Fable.React#34
alfonsogarciacaro wants to merge 6 commits into
masterfrom
compatibility-with-latest-fable-react

Conversation

@alfonsogarciacaro

Copy link
Copy Markdown
Contributor

NOTE: This is just a test I did locally, I haven't published Fable.React with the new namespaces yet.

Following discussions in fable-compiler/fable-import#73, I've started to split Fable.Import.Browser. And I thought it could be a good opportunity to drop the Fable.Import (and Fable.Helpers) prefix and have smaller packages in their own repository for easier maintenance. In Fable.React, I'm dropping most of the bindings generated with ts2fable and restructuring the files.

I thought it would be possible to have namespace/module alias to minimize the number of changes, but it seems these alias cannot be made public. So for now I'm just trying to raise a message with the new name. Does anybody know a better way?

What do you think? Does this change make sense (see also this discussion) or should we keep the namespaces to prevent too many breaking changes?

@ncave @et1975 @MangelMaxime @Zaid-Ajaj @Nhowka @nojaf @forki

@MangelMaxime

Copy link
Copy Markdown
Member

I do like the distinction between Fable.Import and Fable.Helpers because I know which code is native or no but indeed not sure if this is adding value.

About the new Fable.Browser.Dom it tries to make thing it only binding the DOM but in fact there is more in it like the window object, console, etc. And I don't really like the Dom in the name :).

Because before we could do Browser.console.log but now it would be Browser.Dom.console.log --> "meh" :p

Another question about the naming is: is there any benefits for adding Fable. to the namespace ? If we want something simple I would name the package Fable.Browser and have the top module be Browser.

I know that you added Dom to it because you plan to add another package Fable.Browser.Workers but I would still keep:

  • Fable.Browser with module/namespace Browser
  • Fable.Browser.Workers with module/namespace Browser.Workers

This will limit the API changes and make it cleaner to use prefixed API.

@nojaf

nojaf commented Jan 24, 2019

Copy link
Copy Markdown

Browser.Dom.console.log also seems a bit wrong. Imagine calling this from Express server application.
But that is more about where console should be.

More to the point, I agree with Maxime.

@alfonsogarciacaro

Copy link
Copy Markdown
Contributor Author

These are very good points, thank you! Sorry for the mess, but for a more in-depth discussion about the names in the new Fable.Browser packages I've opened a new issue 😄 fable-compiler/fable-browser#1

@Zaid-Ajaj

Copy link
Copy Markdown
Member

And I thought it could be a good opportunity to drop the Fable.Import (and Fable.Helpers) prefix and have smaller packages in their own repository for easier maintenance

❤️ Finally! having to always open these was always confusing and distracting: one namespace has types so I need Import for the signatures and the other has code so you need Helpers as well (very generic name, too many libraries have it) not to mention opening nested modules: (e.g. Fable.Import.React.Props)

Although I agree with @MangelMaxime that Fable.Browser.Dom doesn't really make sense.

IMO that the following is much cleaner:

  • Fable.Browser as the package name and the namespace (easy to find on nuget)
  • Browser is the auto-opened module to use to work with browser API's when someone opens the namspace Fable.Browser.

Library code:

namespace Fable.Browser 

[<AutoOpen>] 
module Browser = 
  type Document = 
      let getElementById : elementId: string -> HTMLElement

  let [<Global>] document: Document = jsNative

Here is how the consuming code looks like:

open Fable.Browser

let rootElem = document.getElementById "root"

console.log("hello from Fable")

So much easier to think about, you need to work with the dom? just open the browser namespace and you are good to go

Another thing I am not sure of is whether to add the types in a different module (e.g. Types) or just leave them in Browser (as in the example above). Let me explain concretely:
this is the alternative library binding code:

namespace Fable.Browser 

[<RequireQualifiedAccess>]
module Types = 
  type Document = 
      let getElementById : elementId: string -> HTMLElement

[<AutoOpen>] 
module Browser = 
  let [<Global>] document: Types.Document = jsNative

this way, if the consumer code wants to use explicit type signatures, they won't have to open yet another namespace but instead, qualify the type name with prefix Types:

open Fable.Browser

// qualify type names if you need them
let init (doc: Types.Document) : unit = 
   let root = doc.getElementById "root"
   root.innerHTML <- "Hello from F#"


init document // document is available since Browser is auto opened

the nice about this, is that if the user uses many binding libraries with Types, then he can just re-use the Types module that includes the types from a different namespace:

open Fable.React
open Fable.Brower

// use types from Fable.React
let makeStyle (x: Types.ICSSProp list) = 
  // do stuff 

// qualify type names from Fable.Browser
let init (doc: Types.Document) : unit = 
   let root = doc.getElementById "root"
   root.innerHTML <- "Hello from F#"

What do you think @alfonsogarciacaro @MangelMaxime ??

@MangelMaxime

Copy link
Copy Markdown
Member

We should keep the discussion in fable-compiler/fable-browser#1.

This PR is about elmish/react and should not be related to discussing how to name things for Fable.Browser project. :)

@alfonsogarciacaro

Copy link
Copy Markdown
Contributor Author

Superseded by #37

@MangelMaxime
MangelMaxime deleted the compatibility-with-latest-fable-react branch July 20, 2025 14:35
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.

6 participants