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

[WIP] Compatibility with new Fable.React #34

Closed

Conversation

alfonsogarciacaro
Copy link
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
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
Copy link

nojaf commented Jan 24, 2019

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
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
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
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
Contributor Author

Superseded by #37

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