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

Add dynamic properties to decoder #647

Closed
wants to merge 1 commit into from
Closed

Conversation

troibe
Copy link
Contributor

@troibe troibe commented Mar 17, 2021

As mentioned in #646
The additional getAllPropertyNames method doesn't look that pretty in my eyes but apparently no such method exists in standard javascript.

@dmjio
Copy link
Owner

dmjio commented Mar 20, 2021

The delegator is some performance sensitive code, so I'm slightly worried about the Set allocation.

@troibe
Copy link
Contributor Author

troibe commented Mar 20, 2021

True now if the javascript code was aware which prop of the object should be decoded that could be a more performant approach.
However I guess you probably put the actual decoding in Haskell and not on the javascript side for a good reason.

@dmjio
Copy link
Owner

dmjio commented Apr 7, 2021

@developandplay yea, decoding is in Haskell so people can build their own decoders with aeson (or other).

@dmjio
Copy link
Owner

dmjio commented Sep 24, 2021

@developandplay been doing some benchmarks, seems adding this would only result in a +/- 0.1ms penalty. In some cases it is actually faster than the existing code... which is surprising.

https://jsfiddle.net/61sw9de5/

^ you basically move your mouse (or click the button) and it benchmarks the time it takes to accumulate all properties w/ both versions of the code, then displays the improvement (positive or negative).

@troibe
Copy link
Contributor Author

troibe commented Sep 24, 2021

Wow this is super cool. Thanks for revisiting the issue!

@dmjio
Copy link
Owner

dmjio commented Sep 24, 2021

No problem, and thanks for this patch ;)

It does seem that new Set() isn't supported in IE 11 down ...

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set#browser_compatibility

To get around this we could check the document.documentMode property.

https://www.w3schools.com/jsref/prop_doc_documentmode.asp

@dmjio
Copy link
Owner

dmjio commented Sep 24, 2021

Or we can remove the Set, and just use a regular object.

function getAllPropertyNames(obj) {                                                                                                                                                                                                 
    var props = {}; 
    ... 
    return Object.keys(props);                                                                                                                                                                                                         
}   

@troibe
Copy link
Contributor Author

troibe commented Sep 25, 2021

Sounds like a decent alternative.
Seems like that was the way to go when set was not a thing yet.

I would expect overshadowing to work as expected.
However it's probably better to double check that since it's the only reason we were using set in the first place.

Also let's add a comment then that we are only using it to "simulate" a set.
Now I'm curious to see if the performance is any different but I would guess no.

@dmjio
Copy link
Owner

dmjio commented Sep 25, 2021

Merged in b32d796

@dmjio dmjio closed this Sep 25, 2021
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.

2 participants