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

Switch to jsonlite #28

Merged
merged 17 commits into from
Apr 13, 2015
Merged

Switch to jsonlite #28

merged 17 commits into from
Apr 13, 2015

Conversation

yihui
Copy link
Contributor

@yihui yihui commented Nov 4, 2014

This might be a breaking change to the existing widget packages, but I think it should be easy for them to adapt to this change. My personal experience is that, among several other advantages, jsonlite covers the corner cases much better than RJSONIO. The default treatment of data frames also makes better sense to JS libraries (record/row-based instead of column-based). Another bonus point is @jeroenooms is extremely responsive, so we can bug him before he becomes a busy super star :-p

TODO:

@yihui
Copy link
Contributor Author

yihui commented Nov 4, 2014

Cc @jjallaire @jcheng5

@ramnathv
Copy link
Owner

ramnathv commented Nov 4, 2014

Thanks @yihui. I think the rationale behind sticking with RJSONIO was to maintain compatibility with shiny, since widgets are expected to work seamlessly in multiple contexts, including shiny applications.

On the issue of conversion of data frames to JSON, while I agree that row/record based makes more sense, the columns based approach leads to smaller payloads. @jjallaire has authored a HTMLWidgets.dataframeToD3 function that does the unpacking of columns to an array of records.

Once @jcheng5 is comfortable with moving shiny to jsonlite, I would be happy to do the same with htmlwidgets.

@yihui
Copy link
Contributor Author

yihui commented Nov 4, 2014

Here is @wch's shiny PR switching to jsonlite: rstudio/shiny#606 I'm not sure whether or when we will get it done, but personally I'd vote for the switch.

I see the point of payloads, which makes perfect sense. It is possible with jsonlite, and widget authors can opt-in by turning data frames to lists, then use dataframeToD3() if they do not like the default row-based representation. With RJSONIO, there is no way to opt-in if we do want the row-based JSON data.

@ramnathv
Copy link
Owner

ramnathv commented Nov 4, 2014

@yihui my vote is for the switch too. But I would want to do it in sync with @wch's shiny PR so that users can get things to work out of the box. One approach for now might be to maintain a htmlwidgets-jsonlite branch that works in conjunction with the shiny-jsonlite branch so that more advanced users can take it for a test-ride.

@jjallaire @jcheng5 any thoughts on this?

@yihui
Copy link
Contributor Author

yihui commented Nov 4, 2014

That sounds good to me. FWIW, I'm not saying this is an urgent issue. I just feel it might be a better option in the long term.

@jcheng5
Copy link
Collaborator

jcheng5 commented Nov 5, 2014

The long encoding of dataframes is massively less efficient than wide though.

> nchar(jsonlite::toJSON(cars))  # long
[1] 1144
> nchar(jsonlite::toJSON(as.list(cars)))  # wide
[1] 313

It's a single JS function call to transform from wide to long, I think it's far preferable to transport the data efficiently and then transform it on the client side.

I think even if/when we move to jsonlite in Shiny, I'd want to discuss more about whether we should change to long-encoding of dataframes, especially when it breaks compatibility with existing output bindings.

(Update: Sorry, had pasted in the example)

@jcheng5
Copy link
Collaborator

jcheng5 commented Nov 5, 2014

That said, RJSONIO does wide but sticks in tons of meaningless whitespace padding even with pretty=FALSE :(

@jcheng5
Copy link
Collaborator

jcheng5 commented Nov 5, 2014

UGH

> nchar(RJSONIO::toJSON(cars, pretty=TRUE))
[1] 626
> nchar(RJSONIO::toJSON(cars, pretty=FALSE))
[1] 829

@jeroen
Copy link

jeroen commented Nov 5, 2014

The row based json is pretty much the standard though. Even when performance should be a concern (e.g. mongodb, crossfilter or web-apis), most applications seems to use row-based encoding because it is much more flexible, especially when data include missing or nested nested fields. So if you want smooth interoperability, I think the row-based format is preferable. Whether that weighs up to some network overhead depends on the average size of your data I guess.

If you are worried about bandwidth, also take into account that most http uses gzip, which makes the overhead significantly smaller:

tmp1 <- tempfile()
tmp2 <- tempfile()
writeLines(jsonlite::toJSON(cars), gzfile(tmp1))
writeLines(jsonlite::toJSON(cars, dataframe = "col"), gzfile(tmp2))
file.info(tmp1)$size
file.info(tmp2)$size

@jcheng5
Copy link
Collaborator

jcheng5 commented Nov 5, 2014

gzip doesn't work on websocket connections (yet), I only wish...

@jeroen
Copy link

jeroen commented Nov 5, 2014

Ah I didn't know that, good to know. But it seems like the proposals for ws compression are ready and being implemented, so that will likely solve itself, soon-ish :)

@yihui
Copy link
Contributor Author

yihui commented Nov 5, 2014

I just added an experimental commit (a7fdb4d) that allows package authors to customize the toJSON() call: the arguments are passed via the attribute TOJSON_ARGS. The reason that I need this is that often times I want auto_unbox = TRUE. For example, it will be terrible if the user has to write

list(order = list(
  list(jsonlite::unbox(1), jsonlite::unbox('asc')),
  list(jsonlite::unbox(2), jsonlite::unbox('desc'))
))

to get {"order":[[1,"asc"],[2,"desc"]]} for the order parameter of DataTables (http://datatables.net/reference/option/order).

Or we can even make the JSON conversion function customizable via, say, an attribute JSON_CONVERTER (with a reasonable default). Then htmlwidgets do not need to take care of the choice of the JSON converter, and package authors can use whatever R package/function they want.

@ramnathv
Copy link
Owner

ramnathv commented Nov 5, 2014

This is a nice idea @yihui. Let me think through the potential consequences of providing this flexibility.

@jcheng5
Copy link
Collaborator

jcheng5 commented Nov 5, 2014

@yihui, a7fdb4d breaks Shiny compatibility.

@ramnathv
Copy link
Owner

ramnathv commented Nov 5, 2014

The to_json function was built to mimic pandas.to_json.

@yihui
Copy link
Contributor Author

yihui commented Nov 5, 2014

@ramnathv That is nice, but I'm wondering how can one actually make use of it if we do not have control over the JSON encoding function.

@ramnathv
Copy link
Owner

ramnathv commented Nov 5, 2014

@yihui I was merely clarifying the origin of the orient argument. to_json was built as a wrapper around RJSONIO to maintain shiny compatibility and provide jsonlite like features, but if we switch to jsonlite, we would not longer require to_json as you correctly indicated.

@yihui
Copy link
Contributor Author

yihui commented Nov 5, 2014

@ramnathv Got you 👍

@yihui
Copy link
Contributor Author

yihui commented Apr 10, 2015

@hafen Please see the one-liner fix at bokeh/rbokeh#56

@smartinsightsfromdata I'll take a look at rhandsontable soon.

@jrowen
Copy link

jrowen commented Apr 10, 2015

For rhandsontable, I think the JSON.parse calls in rhandsontable.js are causing the problem. If I remove these, the widget renders. In the R code jsonlite::toJSON is being used to pass some data and config parameters to the widget. With this update, maybe there's now a better way to handle this.

@bwlewis
Copy link
Contributor

bwlewis commented Apr 10, 2015

This branch seems is working fine with threejs so no problem from me if you merge this.

The threejs widget uses the rjson package on its own in a few places. I will work on moving that over to jsonlite this weekend, and expect to push threejs to cran maybe Sunday night.

@jeroen
Copy link

jeroen commented Apr 11, 2015

@yihui I think @jrowen's problem is what I was afraid of with verbatim json includes. When the user legitimately wants to include a json string to parse it manually on the client, you will get unexpected results when the json gets included verbatim.

@jrowen Try using unclass(jsonlite::toJSON(x)) in your code.

@bwlewis
Copy link
Contributor

bwlewis commented Apr 11, 2015

OK, I spoke too soon. Something else is wrong with this PR, perhaps unrelated to the use of jsonlite. My threejs shiny examples are broken, with undefined arguments in the JavaScript render function. Those examples work fine with the released version of htmlwidgets.

I'll try to track down exactly what is causing this problem, but please hold off merging this.

@jrowen
Copy link

jrowen commented Apr 11, 2015

@jeroenooms The unclass suggestion fixed the error. Thanks.

I'm running now into an "Input to asJSON(keep_vec_names=TRUE)..." message when passing a data.frame to the widget using unclass(jsonlite::toJSON(data, na = "string")) (no message with matrix).

@yihui
Copy link
Contributor Author

yihui commented Apr 11, 2015

@jeroenooms @jrowen Wait a minute. I guess we are heading to the wrong direction by using unclass() -- we should have just removed JSON.parse() instead. Let me experiment with it and get back to you. Jeroen, I believe this is a perfect case why json_verbatim = TRUE is a better default, contrary to what you were thinking, because there is really no need to double encode the data object, and double decode it. Encoding and decoding should be done only once.

@timelyportfolio
Copy link
Collaborator

@bwlewis might your issues be caused by auto_unbox?

@timelyportfolio
Copy link
Collaborator

@ThomasSiegmund , you might be interested in this discussion. Could you try your D3TableFilter with this pull?

devtools::install_github(c('jeroenooms/jsonlite', 'rstudio/shiny', 'yihui/htmlwidgets@jsonlite'))

@yihui
Copy link
Contributor Author

yihui commented Apr 11, 2015

@bwlewis I sent a PR to you and tested your shiny examples under https://github.com/bwlewis/rthreejs/tree/master/inst/examples and other static examples in the documentation. I did not see any issues. Please let me know you still have any problems (a reproducible example will be helpful).

@jrowen I verified what I was thinking: your use of toJSON() + JSON.parse() was mostly due to the fact that RJSONIO did not work well enough, so you had to hack around using jsonlite. Now you don't need the dark voodoo any more, and can enjoy a clean interface between R and JavaScript without taking care of encoding/decoding the data manually.

@bwlewis
Copy link
Contributor

bwlewis commented Apr 11, 2015

@timelyportfolio @yihui
Thanks Yihui, your threejs PR is merged and works great. No issues anymore for me!

@yihui
Copy link
Contributor Author

yihui commented Apr 11, 2015

@bwlewis Great!! I think @timelyportfolio was correct that auto_unbox was the reason. We changed the defaults of jsonlite::toJSON() by using a wrapper function shiny:::toJSON(), and auto_unbox = FALSE was changed to TRUE.

@timelyportfolio I tested @ThomasSiegmund's D3TableFilter package, and the examples still work fine.

@ThomasSiegmund
Copy link

Hi all,

I can confirm that D3TableFilter works fine. I get a warning about a named
vector not beeing supported in future versions of jsonlite. Will change my
code accordingly.

Thanks for taking care

Thomas

On Sun, Apr 12, 2015 at 1:29 AM, Yihui Xie [email protected] wrote:

@bwlewis https://github.com/bwlewis Great!! I think @timelyportfolio
https://github.com/timelyportfolio was correct that auto_unbox was the
reason. We changed the defaults of jsonlite::toJSON() by using a wrapper
function shiny:::toJSON(), and auto_unbox = FALSE was changed to TRUE.

@timelyportfolio https://github.com/timelyportfolio I tested
@ThomasSiegmund https://github.com/ThomasSiegmund's D3TableFilter
package, and the examples still work fine.

Reply to this email directly or view it on GitHub
#28 (comment).

@smartinsightsfromdata
Copy link

I've re-tested my widgets:

  • rpivotTable
  • rdatamaps
  • rd3pie

And they all work fine, beside the warning below in rdatamaps (already corrected by @yihui in rhandsontable, so I have an idea where to look to fix this):

Input to asJSON(keep_vec_names=TRUE) is a named vector. In a future version of jsonlite, this option will not be supported, and named vectors will be translated into arrays instead of objects. If you want JSON object output, please use a named list instead. See ?toJSON

Many thanks to everyone: it is really rewarding been part of this community.

@yihui
Copy link
Contributor Author

yihui commented Apr 12, 2015

@ThomasSiegmund @smartinsightsfromdata You just need to find out where the named vector is created, and turn it into a named list instead. Here is the fix I provided to rhandsontable: https://github.com/yihui/rhandsontable/commit/67462eb9156edc480572f2f89eeed7d8587595f6

@jeroen
Copy link

jeroen commented Apr 12, 2015

FYI the new jsonlite is on CRAN already.

@@ -13,7 +13,7 @@ License: MIT + file LICENSE
VignetteBuilder: knitr
Imports:
htmltools (>= 0.2.6),
RJSONIO,
jsonlite,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a version requirement for jsonlite or will any version work?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we should specify that it is the latest version. (>= 0.9.16.9000)

Copy link
Owner

Choose a reason for hiding this comment

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

Going by the CRAN version, this should be jsonlite (>= 0.9.16)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

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.