cloudflare / doca

A CLI tool that scaffolds API documentation based on JSON HyperSchemas.
BSD 3-Clause "New" or "Revised" License
227 stars 36 forks source link

Display of properties does not preserve order #28

Closed Relequestual closed 6 years ago

Relequestual commented 7 years ago

I would like my object definitions to appear in the order found in the schema files. It makes sense to have the definitions for fields to be in the same order as the example object.

I notice the current CF documentation has this issue. It looks like a theme issue from my limited understanding.

@handrews I know objects are unordered, so is this actually a JSON Hyper Schema issue? If we want JHS to be used to generate documentation, then do we need a field order array?

I'm investigating the posibility of using https://facebook.github.io/immutable-js/docs/#/OrderedMap, but really I'm not even sure how to create my own theme... Yes the docs say "fork this project" but then what? I don't want to publish it to npm... It's like I should already know what to do after I fork it.

Relequestual commented 7 years ago

I notice that the example object preserves the order of the properties. I believe it's down to https://github.com/cloudflare/json-schema-example-loader/blob/35d0b38488c6f34a34031cb61e3dc868c0450396/lib/example-data-extractor.js#L68, right?

handrews commented 7 years ago

+1 to this, we should be loading into ordered maps or whatever the most convenient implementation is (I'm doing this on the Python side with OrderedDict, but that's not helping for documentation :-)

Relequestual commented 7 years ago

This issue may need to be ignored... the map might actually already be ordered by default...

It seems that by adding examples, my example and properties are coming out in the order found in the schema.json file. Which is good, but strange given that the properties list for example objects on the actual CF API docs do not match in terms of ordering. I've tried it several times, and it's consistent, although it may just be caused by something else... I really don't know =/

tajo commented 7 years ago

Hm, I am not sure if we can actually guarantee the order based on properties since JS objects don't guarantee the order.

4.3.3 Object An object is a member of the type Object. It is an unordered collection of properties each of which contains a primitive value, object, or function. A function stored in a property of an object is called a method. - ECMAScript specs

handrews commented 7 years ago

@tajo at least in python this is handled by reading the JSON or YAML into a special OrderedDict class in place of regular dict (python dict == javascript object more or less). The load methods in both libraries have hooks to substitute a different object for the standard one- I imagine someone must have done this for JavaScript by now.

tajo commented 7 years ago

An object is an unordered set of name/value pairs. http://json.org/

If you need to preserve the order in JSONs (or JS), you should use an array.

handrews commented 7 years ago

We're not talking about the data model here, we're talking about presentation. If I list object members in a schema in a particular order, I want the documentation to consistently show those object members in that same order.

Native JS objects don't do that, but there are these things called libraries, you know :-)

Example (I have not even vaguely checked that this is suitable for us, just pointing out that such libraries exist):

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

Relequestual commented 7 years ago

I agree that an object is an ordered list of key value pairs. It's usually phrased that you should not expect the order to be preserved, especially when looping. I would guess this is for speed. However as @handrews points out, this is a presentation issue, and there IS support for preserving order of JSON objects.

As I said in my earlier comment, one of the packages is already doing this by looping through them using a different function. Also, it seems that when the examples are included, the order is preserved... at least for me, consistently.

Relequestual commented 7 years ago

I've confirmed multiple times. If there is an example object given, all the key / value pairing is the same as the original json document, and consiently displayed. SO, it's just ouputting the json document in the right order to resolve this issue (for me).

I'm not sure if you expect someone to use doca without having an example object, but yet list the properties. If you might, then this is still an issue that should be addressed.

tajo commented 7 years ago

Well, I did a bit research. I haven't found any JS library that would solve this. Just a lot of negative answers.

Let's say we found a library that is parsing json files and preserves the order of objects. (It shouldn't be technically called "JSON parser" since it doesn't follow specs). Our json-schema-loader is just a wapper around json-schema-ref-parser, so we don't actually have access to the parsing logic right now. We would have to find a different json-schema-ref-solver library. I'm pretty sure that none of them is using advanced datastructures as Immutable.OrderedMap that would be needed to make this possible. In other words, we would have to rewrite every single library that will touch the schema because they all use plain JS objects.

So, we would have to implement our own JSON parser (which is really hard and will be slow compared to the native v8 parser) and then we would have to write our own JSON schema resolver library (which I already tried once and really don't want to do it again). That's like weeks of work.

I understand that it can be important to set the specific order but the only reasonable technical solution is to add an extra array that will list that order.

handrews commented 7 years ago

Let's put this on hold. At some point we will be looking at which JSON Schema libraries we use as new drafts come out. It may be easier to deal with then.

There are other options we can consider, such as reading the file once and building up that ordering array automatically, but that is more effort than I want to spend on this right now.

Relequestual commented 7 years ago

Thanks for digging a little.

Having a further think on this, maybe the only reasonable solution would be to extend the functionality of the doca config.js file.

An object with keys of the schema files, with an array of the order of the properties keys. schema_name => array [ order of properties ]

I mean, given I'm generating the schemas, and I have the information, this doesn't seem like an unreasonable (or as hard to implement a) solution.

Relequestual commented 7 years ago

OK. So I have spent a LOT of time working on this... I have a solution for if you include in your json an array of properties in the order you want them.

https://gist.github.com/Relequestual/95a74a0a81aa20e8536fcc9e083ef0f2

I'm not sure if this is the best solution, and I feel it probably wants to be solved upstream (Possibly in the example loader? That's where the morphing and setting all_props, required_props, optional_props, is done right?)

Also I haven't catered for if the properties_order field is omitted, and obivoulsy it still needs to function in that situation.

tajo commented 7 years ago

Yea, that's the right direction if you want to solve this issue. Agree that is should be more upstream and I think it can be simplified too.

handrews commented 7 years ago

I'm still really strongly against maintaining a separate array for ordering. If we need to work out an ordering list somewhere, then at the point where we read the file, we should read the field ordering and construct any necessary ordering array.

If no one has solved this problem (which is baffling to me- it's not hard and it's been solved in Python for many, many years now) then we should solve it. If necessary, we can read the JSON into Python, use Python to generate the necessary arrays, and then pipe it into JavaScript. That's a horrible kluge but it's orders of magnitude better than manually maintaining a separate array.

I'm fine with using an array within JavaScript code, but I am absolutely not going to support requiring end-users to maintain such arrays. Think about the larger schemas with many levels of nesting. You'd need to maintain arrays all over the place for each object. We're not doing that.

Relequestual commented 7 years ago

@tajo Great. I'm going to use this solution in our theme for now (even if it's not optimal, because I need to move on for now), and change it when a solution is implemented more up stream.

Relequestual commented 7 years ago

@handrews At first, I dissagreed with you, but after reading, I partially agree. It would indeed be nuts to maintain many many arrays nested all over the place.

However, with Perl, it's a LOT easier to include an array of ordered field names than to order the JSON output. It's not impossible, but pretty tricky.

I see no harm in supporting it as an option, to override the given order, as I expect in other langauges this may also be preferable.

If it presents significant overhead development wise (upstream), then I'll be happy to reconsider.

handrews commented 7 years ago

From a conversation a while back with @epoberezkin, most JavaScript implementations default to preserving the order of objects unless they are very large (larger than we need to worry about right now). This is not any sort of standards conformance thing, just a de-facto behavior.

The problem here is that json-schema-example-loader transforms structures in various ways, and the ordering gets lost in the process. It may be possible to avoid that, in which case we should do so.

I'm not going to remove "blocked" just yet as this needs some investigation- if the problem is that we're actually running afoul of fundamental lack of support for remembering order, this will still face the same dilemma it does now.

Relequestual commented 7 years ago

Apprecaite the update on this. I'm happy with our solution for now - It works for what we need but wouldn't work for everyone. Really hoped to be pushing our API docs sooner, but it's delayed for various reasons.

Codelica commented 7 years ago

Sorry to revive this, but was curious of the current state of the issue. Which would be most accurate?

Thanks..

handrews commented 7 years ago

@Codelica I expect that when we follow through with the ROADMAP.md work, this will be better. Part of it is that we will do less transforming of the schema, so JavaScript's tendency to iterate at least smaller objects in the order they were read will make this less likely to be a problem.

It will also give us an easier way to hook in extension keywords, whether they are proposed standards or just something that we want for doca.

I don't have a timeline on this. There is related work going on- a few things have been published here, and I have some other things in flight, but there aren't set milestones, I'm afraid.

Codelica commented 7 years ago

Thanks for the info.

handrews commented 6 years ago

@Codelica

This set of packages is being deprecated in favor of the JSON Schema Tools monorepo. Contributions are still welcome here but most work will be done on the new packages.

The new back-end does a lot less re-arranging, so it's more likely to preserve order. It depends a lot on how much you use allOf, I think (and of course, it's dependent on the JavaScript implementation's handling of object ordering). I've filed cloudflare/json-schema-tools#4 to track further necessary work.

Codelica commented 6 years ago

Thanks for the update. In the end we just had too many custom needs, so I ended up rolling my own doc output with the help of json-schema-ref-parser, which actually worked quite well and gave us more control over ordering and other things -- at a cost of course ;) Will check out your new collection though... thanks!

handrews commented 6 years ago

@Codelica no worries- I knew that we weren't moving fast enough here to keep everyone. Hopefully with the new codebase sticking more closely to the JSON Hyper-Schema spec we can build up a more active community.