Add $id values into remote schemas

[ChALkeR]

I believe that that data should be present in the files themselves, not in an external index.js file.
Implementers should not be expected to even look there.

This way, index.js is not required to understand what is going on, and e.g. a static schema file analyzer can pick up those schemas and link them together.

A further improvement would be to specify $schema in all the tests, but that's out of scope of this PR.

[karenetheridge]

I do not feel this PR is required -- and it should not be applied in totality, as I describe below.

Implementers should not be expected to even look there.
This way, index.js is not required to understand what is going on

It isn't necessary for implementations to do this. The mere act of loading a document via a $ref is enough to give that document a canonical uri, if there was not an $id present in the document. The specification does indicate this (in the sections discussing how a canonical URI is determined for a schema resource).

I think we should keep some tests in the test suite where a remote ref does not have an $id, to ensure that this functionality works as expected. Some remote resources can have $ids (and indeed we should test what happens when the $id is different from the URI actually used to load the document - as both URIs should be available for referencing the document), but we should have some resources without $ids as well.

[ChALkeR]

@karenetheridge that's not the point of this PR.

Currently, there is nothing in the tests (or remotes) dir that links http://localhost:1234/ to the remotes folder.
The only place that performs that binding is index.js, which shouldn't be even part of the testsuite, but is a tooling around it.

The mere act of loading a document via a $ref

How exactly can that loading be performed without some way of binding those json files to http://localhost:1234?

Currently, the testsuite supports only external manual binding (like index.js did).
With this PR landed, a static no-configuration analysis based purely of *.json files will also work.
The current way of loading is not disturbed by this PR.

[karenetheridge]

I believe the intent was to spell this out in a README. If it's not there in master, there should be a PR for it (I'm not in a place where I can look right now). That is -- wording something like "the files in remotes/ are intended to be made available to the implementation with a base URI of "http://localhost:1234/". We need language like that anyway, because it is not reasonable to ask an implementation using the test suite to pre-parse all the files in remotes/ to determine what URIs to make those files available under.

I remain firm that an $id keyword should not be added to every file in remotes/. Some of them is fine, but we should be testing its absence as well.

[ChALkeR]

I remain firm that an $id keyword should not be added to every file in remotes/. Some of them is fine, but we should be testing its absence as well.

Ah. I see the concern now. I'll think what can be done about that...

[Julian]

I didn't review this PR (yet?), but the index.js that is here is completely unrelated to the test suite (it was mistakenly added, and will be removed as part of #314 or earlier given how often it confuses contributors).

So literally ignore it if it's causing issues.

[Julian]

(Initially though just glancing, yeah, not sure I see why it matters whether these schemas have or don't have IDs declared -- I think you're possibly yourself getting confused by that index.js doing things :/)

[ChALkeR]

@Julian Tests expect files from the remotes dir to be bound to http://localhost:1234/.
Nothing else in this repo mentions http://localhost:1234/ except for index.js.

That is the issue, not index.js itself.

The problem is that test suite expects refs to http://localhost:1234/ to resolve to the remotes dir, but that is both done outside of the testsuite (as you mentioned -- index.js is not a part of the testsuite) and completely undocumented.

Adding $id to the remotes (as this PR does) would have fixed this problem.

[Julian]

jsonschema_suite serve is in theory the "supported" way to host that directory unless you do so fully custom.

Obviously that should be documented yes :) that I think @karenetheridge filed as #364

I don't think adding $id really fixes or doesn't fix anything, the real issue is as you say you need to know to do so (either know to use serve and have an extra dependency installed, or know to parse the remotes directory and mount the schema, as index.js does. Whether the IDs are implicit or explicit though doesn't seem too important personally, but open to discuss certainly).

[Julian]

Going to close this I think due to the above, but if you think that's incorrect or want to discuss further obviously we can. I think #364 should hit how to tell users of the test suite that they need to load these though.