Replacing own JSON implemementation with Crockfords JSON parser
ff6347 opened this issue · 35 comments
I would really like to removing this JSON decode/encode functions and replace them with Douglas Crockfords JSON parse/stringify maybe with an alias for stringify and parse or removing decode/encode.
I would go with the naming loadJSON() and saveJSON() to be aligned with Processing and p5.js. I don't care which implementation it has in the background, as long as it works (our current one has at least one bug: #148 ).
@fabianmoronzirfas, is Crockfords implementation ExtendScript compatible?
Yes it is. I think we also should keep the normal JSON object around and not only have dedicated functions for writing to disc and loading from disc or url
+1 for Crockfords' JSON implementation!
+1 to add loadJSON() and saveJSON()
+1 to keep or add a new decode/encode JSON function. But we definitely need them also without the save/load from hard disc mechanism
Ok, so what do we have now?
loadJSON()saveJSON()encodeJSON()decodeJSON()
Is that correct?
Yes and keep the JSON object in global scope I would say and so JSON.stringify and JSON.parse are also available. Might be the case that people are using this in P5. This is browser standard.
Wait, but stringify() and parse() are equivalent to encodeJSON() and decodeJSON(), right?
I'm still not sure about keeping the JSON global object. I see no real advantage in keeping it (except for catching those copy paste JSON.stringify()/JSON.parse() commands, but I don't think it should be basil's job to polyfill standard Javascript functionality). But I am open to your opinions.
Hm, yes, okay now I understand the reasoning a bit better.
But what about encode and decode? They will be kicked out in favor of stringify and parse?
++ on kicking them
Ok, fine with it as well. But we'll need to implement this with basil 2.0 then, since it will break backwards compatibility.
Cool. Should we support JSONP like p5.js does? https://p5js.org/reference/#/p5/loadJSON (btw I'll be AFK for the next few days)
No to JSONP! :)
Good call. :)
Why can’t we have both? They could both point to the same function. P5 peeps happy, DOM peeps happy. There is an obvious use case for both here but generally if there is a crossroad we should favor a translation from processing right?
if there is a crossroad we should favor a translation from processing right?
IMO we should be close to Processing and P5.js where we can but still keep in mind that we are in InDesign.
My favorite solution is as @b-g proposed:
loadJSON()
saveJSON()
JSON.stringify
JSON.parse
Ok, I have implemented Crockford's JSON.stringify() and JSON.parse() now, you can check it out in the json branch. Here's a quick script for your testing pleasure:
// @include ~/Documents/basiljs/basil.js;
function draw() {
var url = "https://itunes.apple.com/search?term=basil&entity=song";
var myString = loadString(url);
var data = JSON.parse( myString );
inspect(data, {maxLevel: 3})
var jsonString = JSON.stringify(data);
inspect(jsonString);
}Now, I need to implement loadJSON() and saveJSON().
One question about that, @fabianmoronzirfas & @b-g :
loadJSON() should be a wrapper of JSON.parse() with the additional smartness that it can load urls or files directly.
saveJSON() should be a wrapper of JSON.stringify() with the additional smartness that it saves the JSON string as a file.
Both correct?
Awesome 🚀. I'll take it for a test ride tomorrow. Which version of the json parser did you use?
Super!
loadJSON()should be a wrapper ofJSON.parse()with the additional smartness that it can load urls or files directly.
✅
saveJSON()should be a wrapper ofJSON.stringify()with the additional smartness that it saves the JSON string as a file.
✅
Which version of the json parser did you use?
Should be the most recent one. The reason why I implemented that now is that I tried the above API sample in my class yesterday and it did not work with basil's old (current) JSON parser. With Crockford's implementation it works now.
Cool. Could we extract the json2.js part into its own file so we can update it automatic?
I don't think that's necessary.
As the ExtendScript/ES3 specification does not update, I think this implementation just keeps on working. Plus, there are some changes I needed to implement (taking out the comments, insert the basil error function and make it not break with InDesign objects), so it would not really be feasible anyways, I guess.
But the latest commit on that file was 9 month ago. So there might be bug fixes or security issues with json
While this might be true, I think it will be more work to implement an automatic updating than just updating this by hand every once in a while (last change to the actual code was more than 2 years ago).
Having said that, if you are really motivated to implement an automated solution, I'm fine with it. I myself have neither the skills nor the time to do so. So, I will first prepare a manual implementation, and if you want to take this further afterwards, please feel free to do so.
I took a different approach to reviewing this today. Started with writing some tests. You can see them here. https://github.com/basiljs/basil.js/blob/json/test/json-tests.jsx
What is wired is the following section:
testJSONFromStringWithStrangeResult: function(){
$.write("JSON.parse('1.000000000000001') should give a result of ");
var parsed = JSON.parse('1.000000000000001');
$.writeln(parsed);
$.writeln("assert(JSON.parse('1.000000000000001') === 1.000000000000001) does...");
assert(JSON.parse('1.000000000000001') === 1.000000000000001);
}Actually this test should fail. The result is:
------
Running DataTests
----
Running testJSONFromString
PASS
PASS
Basil.js Error -> JSON.parse(), text is not JSON parseable.
PASS
----
Running testJSONFromStringWithStrangeResult
JSON.parse('1.000000000000001') should give a result of 1
assert(JSON.parse('1.000000000000001') === 1.000000000000001) PASSES?
PASS
--
2 test cases run
4 assertions run
4 passed
0 failed
------------
2 test cases run
4 assertions run
4 passed
0 failed
Actually not wired. InDesign can only do 1.00000000000001 one more and it becomes 1
The new JSON functions have been working great! Wanted to ask if others thought it worth keeping JSON.decode()/JSON.encode() around as mirrors for the new standard JSON.parse()/JSON.stringify()? Even though old code will break with the removal of b. – is it worth a few lines to keep it functioning? Maybe there's a nice way for it to silently work (without any documentation just for backwards compatibility)?
Guessing CSV should get the same treatment, parse()/stringify()+loadCSV()/saveCSV() or loadTable() (p5.js way)? It's still using encode/decode... could be back burner things to adjust, but at the least probably worth changing to parse()/stringify().
-1 keeping the decode functions
LoadTable in p5 is pretty huge. I skimmed through the code recently. Do we need this?
I agree we don't need loadTable() – just want to keep the language consistent = maybe CSV.decode()'/'CSV.encode() should become CSV.parse()/CSV.stringify() too? Adding a loadCSV() (like our loadJSON(), which is great) could be helpful for passing a file/url. Guess so much changes in 2.0 it's ok to drop the decode/encode wording
ask if others thought it worth keeping JSON.decode()/JSON.encode() around as mirrors for the new standard JSON.parse()/JSON.stringify()?
JSON.decode() and JSON.encode() have been removed already, haven't they?
maybe CSV.decode()'/'CSV.encode() should become CSV.parse()/CSV.stringify() too?
Yes, that would be good. 👍
Sounds good – working on this right now (and adding simple loadCSV() + saveCSV() to match our JSON functions.
decode()/encode() had been removed.. by 'keeping' I meant to bring them back. But enough has changed, we'll just let them be replaced.
Question: We have the delimiter() function, which doesn't seem necessary. Wouldn't it be better to continue using , as default and add an optional 2nd param to the parse() + stringify() for a custom delimiter?
Currently:
var rawText = loadString("file.txt");
CSV.delimiter("\t");
var data = CSV.parse(rawText, "\t");
Could be:
var rawText = loadString("file.txt");
var data = CSV.parse(rawText, "\t");
Allowing (soon):
var data = loadCSV("file.txt", "\t");
Question: We have the delimiter() function, which doesn't seem necessary. Wouldn't it be better to continue using , as default and add an optional 2nd param to the parse() + stringify() for a custom delimiter?
Yes, sounds good. We should remove the delimiter() function (it sets the delimiter globally right?).