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] Fable compatibility #36

Merged
merged 19 commits into from
Oct 1, 2019
Merged

[WIP] Fable compatibility #36

merged 19 commits into from
Oct 1, 2019

Conversation

krauthaufen
Copy link
Collaborator

@krauthaufen krauthaufen commented Oct 1, 2019

PR for fable branch. (#32)

Most things are working and there's a super-simple demo in src/Demo/Fable that can be started via build -t WatchFable (watcher on localhost:8080)

Remaining problems:

  • AbstractDirtyReader<'T> can't type-test for 'T so some kind of Kind for AdaptiveObjects will be needed.
  • Dictionary/HashSet most likely not hash-based, so caches may be broken

@krauthaufen
Copy link
Collaborator Author

everything working now 🤘

@krauthaufen krauthaufen merged commit b56f1e2 into master Oct 1, 2019
@dsyme
Copy link
Collaborator

dsyme commented Oct 2, 2019

This is amazing. I've never worked on a library that has both .NET and Fable support, including CI. The future is here!

@granicz
Copy link

granicz commented Oct 4, 2019

Just a note here: I think it's pretty shortsighted and discriminative to litter general-purpose libraries with code that works around shortcomings of any particular transpiler. I have the same observation about Elmish.

@krauthaufen
Copy link
Collaborator Author

So what's your suggestion then?
I agree that it's not beautiful but what are our alternatives?
Should we just ignore the fact that fable has become an important F# compiler too?
Should we maintain two versions of the code in separate repos?
Should we just hope that Fable will support all of this someday?

I personally think that the necessary changes are managable and therefore decided to do it the way I did.
Would you have the same objections if we used #if NETCORE in several places?

@granicz
Copy link

granicz commented Oct 4, 2019

It is your responsibility as a library author to decide, but right now this makes it a definite no-go for me. You are also using an FSharp namespace that others might take as an indication that they are dealing with "official" or "F#-recommended" code (and I think it's premature to stage these ideas in that namespace too), yet you are biased and discriminative in your choice of a transpiler - which should have no bearing on the code anyhow, effectively ignoring part of the community for whom that choice is different or implicitly directing people to your choice.

Your opinion is that Fable "has become an important F# compiler" is more of an indication that you haven't looked at other alternatives. WebSharper, for instance, would not require littering up the code base with silly special cases, all you need is providing the necessary proxies in a separate add-on library.

This is a sensitive topic, because you are obviously targeting web applications but you are doing it without doing due diligence on what exists already and without consulting with other library authors who are affected. And as they say, that's a pretty unfriendly way of doing things. WebSharper.UI (and maybe others) are available for an alternative web ecosystem. Say, if that was made to work outside the web as well, would you or any Fable user mind calling it FSharp.Data.Adaptive? I think so.

The namespace and due diligence aside, you are already maintaining two versions of the code, and I don't see how anyone can think that's an acceptable solution here - just look at mess that had to be merged to make that happen. If you care about Fable support so much, you should consider renaming this library to reflect that.

@dsyme
Copy link
Collaborator

dsyme commented Oct 4, 2019

Let's break this discussion into pieces.

First, I refer people to the library design guidelines. These are potentially dated, we can discuss, review and adjust.

On the question of FSharp.* libraries supporting transpilation toolchains, I've drafted some initial guidelines. Please let's discuss the principles there, independent of any particular library.

On the question of naming - I am confident the library falls within the guidelines, which have always sought to balance the need for good naming with the need for positive innovation. This is based on:

  • my understanding the importance and generality of the OCaml "incremental" implementation as a central library
  • my reviewing the Aardvark codebase and seeing its utility in action
  • my understanding the performance characteristics of the technique
  • my reviewing Incremental.NET

I can see the case under the guidelines for potentially calling this "FSharp.Data.Adaptive.Experimental" though that's only a "Consider" guideline. (If the "Experimental" is not added then there is really an obligation on myself and @krauthaufen to abandon and delete the use of the FSharp.Data.Adaptive namespace with a 0.0.x version sequence for this library should the library development stop at some point.)

With regard to parts of WebSharper.UI - it is very reasonable to propose moving parts of WebSharper.UI into a separate library in the FSharp namespace. I am not familiar enough with the components to make that call, but if you'd like to propose it I'm open to suggestions or just go ahead and prototype within the guidelines above.

@granicz
Copy link

granicz commented Oct 4, 2019

I just commented on those proposed new guidelines you added, and will move further comments there as the discussion unfolds. The namespace issue you expanded on above to me is governed by this guideline, however:

  • "We need to avoid pollution of the FSharp.* namespace, particularly by unfinished projects."

@krauthaufen krauthaufen deleted the fable branch October 4, 2019 15:11
@dsyme
Copy link
Collaborator

dsyme commented Oct 10, 2019

• "We need to avoid pollution of the FSharp.* namespace, particularly by unfinished projects."

Yes, I know - though see also the next guideline about "incubation and experimentation". When we wrote these we knew there was a tension these two and they need to be considered in balance. As I mentioned, I understand your concern but in balance I'm ok with proceeding.

If the library stagnates, is incomplete or doesn't get used we should rename or delete it. That's not really covered by any existing process - we probably need to pay a full time cleaner! There's history with parts of FSharpx where we worked hard to rename it, it was painful TBH.

The really hard thing is when there are several plausible designs to core libraries that perform essentially the same role and can't be reconciled. Map/Set implementations are like this, as are "batteries included" libraries. Another huge problem is FSharp libraries that have complex dependency chains. I generally prefer these libraries to have almost zero dependencies apart from FSharp.Core and framework.

@krauthaufen
Copy link
Collaborator Author

Hey there,

For me it is just not really important that this library is called FSharp.Data.Adaptive and I would also be okay with just calling it FsAdaptive or something alike if that resolves any concerns...

Regarding completeness and stability I want to mention that most of the code comes from Aardvark.Base.Incremental which evolved over several years now and is also quite battle-tested in real-world applications. Furthermore there are pretty sophisticated tests in the repo validating against a reference-implementation.

The major design choice here is that the system uses lazy evaluation and arguably one could implement a similar system with eager evaluation, so maybe a Lazy suffix would be something we could think about. However most eager systems are called Incremental and the name Adaptive is inspired by Adapton, which also uses lazy evaluation.

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