947694 - Add a JSM with simple APIs for reading and writing JSON files

Status: RESOLVED WONTFIX

(Toolkit :: Async Tooling, defect)

Description

[Gregory Szorc [:gps]]

There are literally dozens of places in the tree where JSON files are written and loaded. This should be a one-liner. However, it isn't. I want to make it one.

While I'm here, I'm also going to throw in an API for reading and writing compressed JSON. AFAICT, nobody in tree is producing compressed JSON (probably because the XPCOM APIs for stream conversion are absurdly complex).

Comment 1

[Gregory Szorc [:gps]]

Attachment: Add JSON.jsm for common JSON operations

This should do it.

I'm not thrilled about the number of buffer allocations to convert to
gzipped data. I'm sure there is a better way, but someone with more
XPCOM string services knowledge will have to tell me about it.

The good news is this suboptimal implementation detail is abstracted
away. So if things are improved in the future, every consumer wins
traparently.

Comment 2

[Gregory Szorc [:gps]]

API design question: Should we be more intelligent and auto-detect encodings based on filename? e.g. if the filename ends with ".gz" we could automagically use gzip compression. This could be bolted on later, of course.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8344305 [details] [diff] [review]
Add JSON.jsm for common JSON operations

Review of attachment 8344305 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/JSON.jsm
@@ +44,5 @@
> +   *
> +   * The written file contains the gzipped header.
> +   */
> +  writeGzippedFile: function (o, path, replacer, space, writeOptions) {
> +    let utf8 = JSON.stringify(o, replacer, space);

This is not utf-8.

@@ +51,5 @@
> +    // buffer allocations.
> +    let raw = this._utf8Converter.ConvertFromUnicode(utf8);
> +    raw += this._utf8Converter.Finish();
> +
> +    let gzipped = CommonUtils.convertString(raw, "uncompressed", "gzip");

This is a terrible API. You're not converting strings, you're converting byte streams, so your API should deal with Uint8Arrays or something similar, not strings.

@@ +80,5 @@
> +    OS.File.read(path).then((data) => {
> +      try {
> +        let decoder = new TextDecoder("utf-8");
> +        deferred.resolve(JSON.parse(decoder.decode(data), reviver));
> +      } catch (ex) {

Isn't this promises stuff supposed to catch your exceptions?

@@ +104,5 @@
> +          a.push(String.fromCharCode(dv.getUint8(i)));
> +        }
> +
> +        let gzipped = a.join("");
> +        let raw = CommonUtils.convertString(gzipped, "gzip", "uncompressed");

Again, shouldn't deal with strings

Comment 4

[Gregory Szorc [:gps]]

I'm not sure how to convert a Uint8Array (or similar) directly to a gzipped Uint8Array. That's why we do some funky conversion voodoo in the realm of JS strings. I would switch in a heartbeat if I knew how. The MDN docs for all this stuff are horribly inadequate, IMO.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8344305 [details] [diff] [review]
Add JSON.jsm for common JSON operations

Review of attachment 8344305 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't really looked at the code. However, we should land bug 854169 quite soon. This will add lz4 compression "for free" (in terms of lines of code), off the main thread. I believe that any new module should build on top of this.

If you want me to look at the API design, feel free to f? me again.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Now that bug 854169 has landed, reading/writing lz4-compressed data is a one-liner.
I suggest WONTFIXing this bug.

Comment 7

[Gregory Szorc [:gps]]

I agree.