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

WebSharper and Metro UI #1391

Closed
giuliohome opened this issue Jul 19, 2019 · 11 comments
Closed

WebSharper and Metro UI #1391

giuliohome opened this issue Jul 19, 2019 · 11 comments

Comments

@giuliohome
Copy link

We have a sample of Metro UI with WebSharper in this repo.
I'm especially interested in the Table component with an Inspector.

The problem is that in a real world Production scenario I will have a client server architecture to retrieve data from the DB. Let's imagine an event from a button to fetch data from the DB
In this case WebSharper has a reactive View with a binding to a Doc.

Long story short, we have found an issue due to the fact that

because metro.js adds some extra functions on Array prototype, and WebSharper's HashSet proxy uses for (... in ...) which now hits the added functions.

Refer to the WebSharper Forum topic related to this matter.
The only workaround at the moment is due to the suggestion:

Because this is more an issue in metro.js (it is not a nice move to add enumerable properties on Array prototype), as a fix, you can include this on top of your RetrieveTrades client renderer function (or move to a separate function and call on every page where you would use metro.js)

// make Array functions added by metro.js not enumerable to avoid WS HashSet issue
JS.Inline "Object.defineProperties(Array.prototype, 
    {
        shuffle : { enumerable : false }, 
        clone : { enumerable : false }, 
        unique : { enumerable : false }, 
        from : { enumerable : false }, 
        contains : { enumerable : false }, 
    })" |> ignore

but I'm afraid it can impact the features of metro style, especially the inspector button that I should be able to click after the table is retrieved and rendered on the page (it is the first most important for choosing Metro UI in my use case).

Is there a solution?

@olton
Copy link
Owner

olton commented Jul 19, 2019

This should not affect the operation of Metro 4.

@giuliohome
Copy link
Author

Awesome!

@giuliohome
Copy link
Author

Well, it is strange, because I can't see the data in the table, while the following code works for a pure html table (and also for Metro UI css/script when the reactive view of WebSharper and the above said workaround is not involved, that is only for a static server rendered page).

(if Array.isEmpty trades then
    p [] [text "No trades selected"]
else
    Doc.Concat [
    div [] [Inspector()];
    table [attr.``class`` "table"; attr.``data-`` "role" "table"; attr.``data-`` "horizontal-scroll" "true"; attr.id "demo-table"] [
    
        yield thead [] [
            tr [] [
                for v in headers do
                    match v with
                    | Numeric k -> yield th [attr.``class`` "sortable-column"; attr.``data-`` "format" "number"] [text k]
                    | Alphanumeric k ->  yield th [attr.``class`` "sortable-column"] [text k]
            ] ]
        for tup in trades do
            yield tr [] [
                for (k,v) in tup do
                    yield td [] [text <| string v]
            ] 

Are you sure that no operations of Metro 4 are affected?
So why would you have added those extra properties on Array prototype in the first place?
Excuse me if I'm not fully convinced that this issue is solved.

@giuliohome giuliohome reopened this Jul 19, 2019
@olton
Copy link
Owner

olton commented Jul 19, 2019

I added these methods because I use them in my projects.

@giuliohome
Copy link
Author

So maybe... if we deactivate thoese methods with the above JavaScript Inline from WebSharper as a workaround while constructing the page, then your project can't build the table as expected... just guessing...

@olton
Copy link
Owner

olton commented Jul 19, 2019

If the problem was in the Table component, then my projects would stop working for me. I am not developing for .NET, so I cannot tell you why your code does not work.

@olton
Copy link
Owner

olton commented Jul 19, 2019

table-array-dis

@giuliohome
Copy link
Author

giuliohome commented Jul 19, 2019

Oh, sorry for the confusion. What I'm trying to say is that your project - in particular the Table component - works fine (actually I love the Table component and the possibility to customize it from the user side via the Inspector), but it sets some extra methods on the Array prototype and that prevents WebSharper (.Net) from building the page... I can't see a solution and I was asking your help. Don't you think that it can be a problem for an external framework that is using such Component? Is it something that affects only WebSharper? I'm sorry I'm not a JavaScript expert but I'm having the feeling that this change on the Array may have quite a broad impact...
Thank you for your replies and feedback in advance

@olton
Copy link
Owner

olton commented Jul 19, 2019

The Table component does not use these extensions for an array. Therefore, disabling them should not affect the operation of the component.

@giuliohome
Copy link
Author

OK, so, if you are right, that means that this issue is solved, but there is another unrelated issue, that at the moment I've not yet discovered, and it is likely a similar incompatibility between the Metro UI script and WebSharper.
What I can prove is that if I don't load the Metro UI script, I see the rows but clearly the Inspector can't work.
Ok, so I close this because I don't have the technical details of the issue and, based on your answer, it is a different thing from those extra methods on the Array prototype.
Thank you for your kind and immediate support.
Maybe I will open another issue in the future.

@giuliohome
Copy link
Author

giuliohome commented Jul 19, 2019

FYI
The latest issue of the missing rows was my fault, I missed the tbody

yield tbody [] [
for tup in trades do
    yield tr [] [
        for (k,v) in tup do
            yield td [] [text <| string v]
    ] ]

But btw - regarding the original topic here - someone is saying that

"Extending the JavaScript Array object is a really bad idea..."

because, they explain:

"Now if someone loads your library in an environment with a new version of the language, your code stomps the built-in. Standard-conforming code now breaks because of your non-standard extension"

and, to be honest, on the other hand, someone else replies that

"using “for…in” with array iteration a bad idea"

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

No branches or pull requests

2 participants