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

Upgrade to Nagareyama #54

Closed

Conversation

alfonsogarciacaro
Copy link
Contributor

This uses Nagareyama instead of fable-splitter to run the tests. In order to run ES2015 modules directly, I'm passing the -r esm option to Mocha.

I made a few minor code changes but some tests are still failing. I know we don't want to make breaking code changes in Fable 3 but some things in SimpleJson depend on Fable or even F# implementation specifics and can easily break so it'd be nice to fix those. For example the FSharpMap and FSharpSet internal representation are changing in FSharp.Core and we're porting the change to Fable: dotnet/fsharp#10188

It's true that testing the type of a dynamic object is tricky in Fable. Nagareyama is removing the restriction of testing generic types (although the type parameters are still ignored) maybe that could help. We could also includes some methods in Fable.Core.Reflection like isMap, isSet, isDictionary`, etc.

elif isBigInt (get key jsThis) then
let bigInt : bigint = get key jsThis
elif isBigInt v then
let bigInt : bigint = !!v
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fails here if the function is compiled as an arrow function. But using the value directly works in either case.

let unexpectedJson = JS.JSON.stringify otherwise
let expectedType = JS.JSON.stringify cases
let unexpectedJson = string otherwise
let expectedType = string cases
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're removing inheritance of union types because of performance reasons so we're giving up some utility methods like toJSON to avoid repetition in every class definition. We can consider to add them again, but using the string operators works here anyways.

@Zaid-Ajaj
Copy link
Owner

Hi @alfonsogarciacaro, thanks a lot for giving this a go. I am running the tests locally and oh boy many stuff broke 😅

We're removing inheritance of union types because of performance reasons so we're giving up some utility methods like toJSON to avoid repetition in every class definition. We can consider to add them again, but using the string operators works here anyways.

This seems to be root problem of many of failing tests especially for the following types:

  • unions (now emitting { tag: caseIndex, fields: array of values })
  • maps (emitting { k, v, left, right, h } which is the internal representation of the type)
  • lists (emitting { head: value, tail: rest or empty }

I understand that you don't want to repeat toJSON() on every discriminated union type but that that for Map<> and list would be very nice, even potentially for other types like Dictionary, HashSet and ResizeArray which I am still trying to figure out why they not being deserialized like they should.

Can you please rebase on master? I have added some functionality to handle the Fable 3 awkwardness without the toJSON function which I was relying on to get sane JSON back when deserializing.

I am not able to run the application in the browser with webpack, is that not possible yet with Fable 3?

Final question, can you help me figure out why the specialized DateTimeOffset serializer utility is not being picked up? It used to emit the local time + offset but now it serialized everything to UTC

@Zaid-Ajaj
Copy link
Owner

I just pushed a new release of SimpleJson that handles the serialization via Reflection in order to control the generated JSON structure with serializing objects into JSON and avoiding the runtime implementation details of the objects (even though the deserializer will be able to handle those anyways)

I believe I made the library Nagareyama-proof by now 😄 just need to run the tests against it 🚀

@alfonsogarciacaro
Copy link
Contributor Author

Wow, I was trying to bring back the missing stuff from Fable 2 to Nagareyama but seems you are faster 🏎️

@Zaid-Ajaj
Copy link
Owner

Having toJSON() implementations is still very nice for unions/records as people would still love to use JSON.stringify instead of the Reflection-based one from here and still get some sane results 😄

@Zaid-Ajaj
Copy link
Owner

@alfonsogarciacaro I made some more changes to allow running nagareyama alongside Fable 2.x and it now on master:

# Run Fable 2.x tests
npm test

# Run Nagareyama tests
npm run test-nagareyama

There are some issues when creating Dictionary<TKey, TValue> and HashSet<TValue> dynamically via reflection where TKey or TValue require structural equality. In Fable 2.x I worked around the issue by using:

let output = System.Collections.Generic.Dictionary<IStructuralComparable, _>()

Where IStructuralComparable would make the comparer function use structualEquals but now that doesn't work anymore and instead I get a generic equals function instead of a structural equality function for comparing keys.

I tried using

let output = Activator.CreateInstance(concreteDictionaryType) |> unbox<Dictionary<IStructuralComparable, _>>

But creating the instance of the dictionary fails saying:

Error while accessing the constructor of System.String

At least something like that where the issue remains of not being able to create the dictionary via reflection, any ideas? 🤔

@alfonsogarciacaro
Copy link
Contributor Author

I am not able to run the application in the browser with webpack, is that not possible yet with Fable 3?

Sorry, forgot to reply to this. Nagareyama is compatible with Webpack, but you need to do a few changes, see MangelMaxime/fulma-demo#43

Also, I've released nagareyama-alpha-008, can you please try with that? Hopefully it solves some of the problems including the dictionary. As unions/records are inheriting Equals/CompareTo again a generic comparer should work, this is what Thoth.Json does although it deals with string keys separately (mainly for performance to make sure a native js Map is generated): https://github.com/thoth-org/Thoth.Json/blob/c52e7ea7ff9918ea0ae82e098cbe2053b3f82e49/src/Decode.fs#L1171

Anyways, you're right that Fable 2 & 3 generate a different hash function for IStructuralComparable, I will have a look 👍

@Zaid-Ajaj
Copy link
Owner

Also, I've released nagareyama-alpha-008, can you please try with that? Hopefully it solves some of the problems including the dictionary. As unions/records are inheriting Equals/CompareTo again a generic comparer should work, this is what Thoth.Json does although it deals with string keys separately (mainly for performance to make sure a native js Map is generated):

Thoth seems to only handle dictionaries where the key is a string, at least it checks that the dictionary type has a full name involving a string as key. Strings work just fine, the problem in my case was when the keys of the dictionary were not a primitive type but something more complex like a discriminated union or even a record.

I was able to solve this problem by cheating a little bit and tricking Fable into generating the right comparer function using dummy types:

let output =
   match keyType with
   | TypeInfo.Union _ ->  Dictionary<Result<_, _>, _>()
   | TypeInfo.Record _ -> Dictionary<{| dummy: int |}, _>() |> unbox
   | _ -> Dictionary<IStructuralComparable, _>() |> unbox

Now all the tests are running both for Fable 2.x and Fable 3 as of SimpleJson 3.13

Nagareyama is compatible with Webpack

As for the changes in webpack, I would really really appreciate if you could reconsider the removal of the use of fable-loader and fable-compiler as a distribution for Fable. Keeping the same structure would make me and many other very happy by not having to restructure every single repository, project and library. Of course, Fable 3 is nice a dotnet tool but it introduces too many levels of indirection if you have to use orchestrate multiple processes to get webpack running with Fable 3 😞

@Zaid-Ajaj
Copy link
Owner

@alfonsogarciacaro I've implemented Fable 3 support in this repo with backward-compatibility with Fable 2.x this is pretty much resolved. Thanks a lot for giving me the head start 😄 I will close this PR and will keep updating the Fable 3 version as more releases become available

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.

None yet

2 participants