Split "extra" modules into their own packages

Zaid-Ajaj commented 5 years ago

Having every possible react-native module just as an "extra" module makes using rn with fable very cumbersome

1) Versioning

Some of these packages depend on a specific version of react-native but here they are all available in this one single package (see for example notice of react-native-fs) and they could be easily incompatible with the current version of rn.
They are not compatible with Femto making it hard to find and use different versions of an "extra" package

2) Discoverability

Using the auto-opened "Helpers" module is the most unhelpful thing to have, because the user will have functions that are globally available and the only way to find them is by having to look in the source code, for example the "helper" of device info:

[<AutoOpen>]
module Helpers =
    let private deviceInfo: obj = importDefault "react-native-device-info"
    /// Gets the API level.
    let getAPILevel () : int =
        deviceInfo?getAPILevel() |> unbox
    /// Gets the application name.
    let getApplicationName () : string =
        deviceInfo?getApplicationName() |> unbox

Please replace instead with DeviceInfo as [<RequireQualifiedAccess>], i.e.

[<RequireQualifiedAccess>]
module DeviceInfo =
    let private deviceInfo: obj = importDefault "react-native-device-info"
    /// Gets the API level.
    let getAPILevel () : int =
        deviceInfo?getAPILevel() |> unbox
    /// Gets the application name.
    let getApplicationName () : string =
        deviceInfo?getApplicationName() |> unbox

3) The use of ts2fable-like constructs

Going through some of the extra packages, I still see U<...> being used instead of proper arguments, for example in here where it is not obvious how the location of the SQLite database can be instantiated from a float

4) Adhoc Bindings

for example react-native-fs now only includes two functions:

[<AutoOpen>]
module Helpers =
    [<Import("default","react-native-fs")>]
    let private fileSystem = obj()
    let deleteFile (uri:string) : unit = fileSystem?unlink(uri) |> ignore
    let getBase64File (uri:string) : JS.Promise<string> = fileSystem?readFile(uri,"base64") |> unbox

This is not helpful for the users of the library and they are better off writing these functions in their application code instead of having two random functions

5) Zero docs

Might be the biggest reason that it is hard to get started with this library, even though it has a really high potential next to electronjs that there is no docs on the "extra" packages

@forki @alfonsogarciacaro @MangelMaxime

forki commented 5 years ago

Oh I'm open for that. I just don't have time. So I'll continue to dump stuff here and if someone wants to step up, then I'm all ears.

Zaid Ajaj notifications@github.com schrieb am Mi., 28. Aug. 2019, 11:56:

Having every possible react-native module just as an "extra" module makes using rn with fable very cumbersome 1) Versioning

Some of these packages depend on a specific version of react-native but here they are all available in this one single package (see for example notice of react-native-fs https://github.com/itinance/react-native-fs#important) and they could be easily incompatible with the current version of rn.

They are not compatible with Femto https://github.com/Zaid-Ajaj/Femto making it hard to find and use different versions of an "extra" package

2) Discoverability

Using the auto-opened "Helpers" module is the most unhelpful thing to have, because the user will have functions that are globally available and the only way to find them is by having to look in the source code, for example the "helper" of device info:

[]module Helpers = let private deviceInfo: obj = importDefault "react-native-device-info" /// Gets the API level. let getAPILevel () : int = deviceInfo?getAPILevel() |> unbox /// Gets the application name. let getApplicationName () : string = deviceInfo?getApplicationName() |> unbox

Please replace instead with DeviceInfo as [], i.e.

[]module DeviceInfo = let private deviceInfo: obj = importDefault "react-native-device-info" /// Gets the API level. let getAPILevel () : int = deviceInfo?getAPILevel() |> unbox /// Gets the application name. let getApplicationName () : string = deviceInfo?getApplicationName() |> unbox

3) The use of ts2fable-like constructs

Going through some of the extra packages, I still see U<...> being used instead of proper arguments, for example in here https://github.com/fable-compiler/fable-react-native/blob/master/src/extra/react-native-sqlite-storage/Fable.ReactNativeSqlite.fs#L22 where it is not obvious how the location of the SQLite database can be instantiated from a float 4) Adhoc Bindings

for example react-native-fs now only includes two functions:

[]module Helpers = [<Import("default","react-native-fs")>] let private fileSystem = obj() let deleteFile (uri:string) : unit = fileSystem?unlink(uri) |> ignore let getBase64File (uri:string) : JS.Promise = fileSystem?readFile(uri,"base64") |> unbox

This is not helpful for the users of the library and they are better off writing these functions in their application code instead of having two random functions 5) Zero docs

Might be the biggest reason that it is hard to get started with this library, even though it has a really high potential next to electronjs that there is no docs on the "extra" packages

@forki https://github.com/forki @alfonsogarciacaro https://github.com/alfonsogarciacaro @MangelMaxime https://github.com/MangelMaxime

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fable-compiler/fable-react-native/issues/54?email_source=notifications&email_token=AAAOANHOL6LPDFJDXHL2KLDQGZDWTA5CNFSM4IRBG4X2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HH3775A, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAOANF5FPF25DVPBNIGNYDQGZDWTANCNFSM4IRBG4XQ .

Zaid-Ajaj commented 5 years ago

So I'll continue to dump stuff here

I understand the positive gesture here but I would suggest to refrain from "dumping stuff" that only adds more low-quality code to the current code base such that only needs to be cleaned up anyways later on when someone wants to actually pick this up.

If you need specific one-off bindings for a function or two, just add it to your application

forki commented 5 years ago

you may call it low quality code. for me it's code thats used in multiple production apps. I don't agree with all your points above but I'm willing to collaborate on if people are actually intending to use it. But so it's way more important for me to get something that is actually working in production hat cleaning stuff up for hypthetical users.

alfonsogarciacaro commented 5 years ago

I guess if someone takes the time to restructure the repo and publish the separate packages, it would be easier for @forki to follow that structure when adding new stuff :)

You can take the fable-browser repo as reference, where the build script checks the release notes of all the packages automatically and republishes the ones with modifications.

forki commented 5 years ago

Well maybe I did not word it properly.

a) most of the stuff in here is important for all the apps that I have. Stuff that's not used in only one app still stays over there

b) I'm fine with splitting it up. It's just that it's more work and the benefits are not clear. If people are committing to actually using it and need it to get started then: sure let's do it

Alfonso Garcia-Caro notifications@github.com schrieb am Mi., 28. Aug. 2019, 14:46:

I guess if someone takes the time to restructure the repo and publish the separate packages, it would be easier for @forki https://github.com/forki to follow that structure when adding new stuff :)

You can take the fable-browser repo as reference, where the build script https://github.com/fable-compiler/fable-browser/blob/master/Build.fsx checks the release notes of all the packages automatically and republishes the ones with modifications.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fable-compiler/fable-react-native/issues/54?email_source=notifications&email_token=AAAOANARTBZKG2VBCLB55TLQGZXTPA5CNFSM4IRBG4X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5K73ZQ#issuecomment-525729254, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAOANAOK75LCOTZGEM53NTQGZXTPANCNFSM4IRBG4XQ .

Zaid-Ajaj commented 5 years ago

you may call it low quality code. for me it's code thats used in multiple production apps.

Using low-quality code in production doesn't automatically qualify it as good code, it just means that you are willing to use low-quality code in your production applications.

I call it low-quality because of the points I mentioned above: invisible (and probably incompatible) versions of different packages that seem "ready to use" once you install this single library but can easily break for beginners who don't know which package goes with which version and to actually get started with any of the packages, it requires going through the actual source code, looking for import statements and installing the relevant packages after checking compatibility (which is what Femto automates) along with other inconveniences with the exposed APIs themselves

I don't agree with all your points above

Issues are made for discussion, I filed this because there is room for improvement so if you have time, please point out what is wrong with the points I mentioned above and in what way you suggest to move forward with the library

If people are committing to actually using it and need it to get started

This is more of chicken-and-egg problem, if it is not easy to use then people will not use it

forki commented 5 years ago

e.g. let's discuss the points

point 1) RN versioning is super hard. Facebook is breaking shit all the time. Every RN update is a puzzle. The versions here are tested together. Femto may make some things easier. But from my experience with RN it will only scratch the surface of what's needed. The biggest problem are the RN native packages themselves.

point 2) this patterm is copied from React proper. maybe it's no longer en vogue. I'm fine with updating it. I don't mind breaking changes.

point 3) shouldn't this be adressed directly in ts2fable?

point 4) those functions will be used in all mobile apps that use photos. RN discontinued support for ImageStore and explicitly encourages people to use this API. So yes they are small and could be copied into every app directly. but that doesn't make any sense.

point 5) yep docs would be useful

again I don't mind if we are splitting things.

fable-compiler / fable-react-native