Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

Kicking the CI

I think it would be better if the server did not send nonstandard JSON (for example, it could send the setting just as raw text to be interpreted as JSON5 on the frontend). If we send nonstandard JSON in response to a settings request, it breaks the expectation of the rest api that the response is valid JSON.

I think it would be better if the server did not send nonstandard JSON (for example, it could send the setting just as raw text to be interpreted as JSON5 on the frontend). If we send nonstandard JSON in response to a settings request, it breaks the expectation of the rest api that the response is valid JSON.

I'm not sure to get it. You would like to do something like this on the server:

settings = json5.load(file)
# Test it is a valid json
settings = json.loads(json.dumps(settings))

And similarly on the frontend to ensure user settings sent are valid json?

So that we will allow json5 syntax but reject specific things like Infinity or NaN?

And similarly on the frontend to ensure user settings sent are valid json?

No. I would send the json5 source as a raw unparsed string, and then parse that string on the frontend as json5. That way what comes over the wire is actually valid JSON.

Bumping from 3.2.x to 3.3.x for consideration. But could also be added to 4.0 to not block the 3.3 release.

@jasongrout I have a trouble with this one. If I got it correctly, the settings attributes in the object returned by the settings REST endpoint will be a raw string instead of a JSON object.

So how do we achieve changing jupyterlab_server API without breaking older JupyterLab versions that will assume that the settings attribute is an object.

Should we test for the content type header?

Should we use application/json5 (xref json5/json5#80)

I was proposing sending settings as a raw string as a breaking change in jlab server.

I was proposing sending settings as a raw string as a breaking change in jlab server.

As there is always a GET request, we could use the following pattern:

sequenceDiagram
  frontend->>server: GET settings
  activate server
  Note right of server: Get format from `Content-Type`
  server->>frontend: Send settings
  Note left of frontend: Store save format from `Content-Type`

The server would keep its current behavior if the content-type is not application/json5. As well as the frontend will keep its current behavior if the server does not answer with content-type application/json5.

What do you think?

I think using the content type would technically work, but it would be the first rest endpoint that didn't return JSON, which I think is a bigger architecture decision. I think I'm -0.5 on sending something other than JSON, just because there is a very standard convention to send JSON for rest endpoints. If it keeps sending JSON, then it keeps being broken. If we stop sending JSON, then there will be lots of confusion about it, I think.

In my mind, I was imagining a breaking change in jlab server where we send different JSON (the raw string), and us depending on it in jlab 4.0, and making a clean cutover then. Iirc, we already have this pattern of sending a raw string, so we'd just be applying that existing pattern here.

Sorry we misunderstood each other. I had in mind to send a raw string as you propose but using the content type to flag that the attribute is a raw string that need json5 parsing. But this is not a great idea (hence our misunderstanding) as by looking at the content type people will think the body they got does not need an additional parsing.

So closing this as the approach will be different.

I had in mind to send a raw string as you propose but using the content type to flag that the attribute is a raw string that need json5 parsing. But this is not a great idea (hence our misunderstanding) as by looking at the content type people will think the body they got does not need an additional parsing.

Ah, I see what you were saying. Yes, I agree that we'd be misapplying the json5 content type in this situation (well, technically not because IIRC json is a subset of json5, but it would still be confusing).