Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load JSON-LD in the same way as JSON #3502

Closed
wants to merge 1 commit into from

Conversation

darobin
Copy link

@darobin darobin commented Oct 23, 2015

The JSON-LD standard requires sending what is effectively JSON content using a different MIME type. Because of that, it is usually required to use a file extension other than .json since that is what is typically used in MIME type mapping in HTTP servers. The IANA-registered file extension is .jsonld.

This patch makes .jsonld documents loadable through require() in the exact same manner that .json files are.

@Fishrock123
Copy link
Contributor

If I understand correctly, this could be done in an npm module? Perhaps that would be a better place for this?

@darobin
Copy link
Author

darobin commented Oct 23, 2015

@Fishrock123 Well, loading .json could be done in an npm module, but the idiomatic way to do it is with require('./foo.json'). Unfortunately there is a standard JSON format out there that pretty much requires a different file extension. This just exposes the existing, idiomatic, built-in Node mechanism to a greater range of JSON content (which is why it's a rather simple patch).

@Fishrock123
Copy link
Contributor

I mean a module that sets Module._extensions['.jsonld'] = Module._extensions['.json'];.

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Oct 23, 2015
@darobin
Copy link
Author

darobin commented Oct 24, 2015

Sure, but again: it seems strange to be able to load some JSON without a module, and to require a module for other JSON.

Don't get me wrong: I think the JSON-LD made the wrong decision to require a distinct MIME type and file extension. I think such approaches should be discouraged. But it's there, it's standard, it seems to be increasingly common, and it seems to me like it ought to be processed identically to JSON so that us developers don't have to worry about superficial differences.

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 24, 2015
@bnoordhuis
Copy link
Member

Has a conclusion been reached on this? Is it something that should be brought before the TC?

@seishun
Copy link
Contributor

seishun commented Oct 27, 2015

I mean a module that sets Module._extensions['.jsonld'] = Module._extensions['.json'];.

I don't think we should encourage writing modules that use internal undocumented functionality.

@jasnell
Copy link
Member

jasnell commented Oct 27, 2015

Hmm... I tend to agree that we shouldn't be requiring modules to use undocumented bits to enable this kind of thing, and being a user of JSON-LD I've been hit by this also. In my case, I simply have my JSON-LD documents use the *.json suffix to avoid the problem. This change is minor, JSON-LD is a W3C Recommendation and there are people using it in real world apps. I'd say LGTM. Let's land it.

@targos
Copy link
Member

targos commented Oct 27, 2015

@@ -0,0 +1,14 @@
'use strict';
require('../common');
var assert = require('assert');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use const here?

@bnoordhuis
Copy link
Member

Couple of nits regarding the test. LGTM if you fix those and squash the commits.

The JSON-LD standard (http://www.w3.org/TR/json-ld/) requires sending
what is effectively JSON content using a different MIME type. Because
of that, it is usually required to use a file extension other than
.json since that is what is typically used in MIME type mapping in
HTTP servers. The IANA-registered file extension is .jsonld
(http://www.iana.org/assignments/media-types/application/ld+json).

This patch makes .jsonld documents loadable through require() in the
exact same manner that .json files are.
@darobin
Copy link
Author

darobin commented Oct 27, 2015

@bnoordhuis Changes applied, squashed, and pushed. Thanks!

@bnoordhuis
Copy link
Member

Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/633/

Before I land this prematurely, does anyone feel this should be discussed more or be brought up at the next TC call?

@targos
Copy link
Member

targos commented Oct 27, 2015

LGTM

@rvagg
Copy link
Member

rvagg commented Oct 27, 2015

I'd like to hold this one off until we get a @nodejs/tsc discussion on this, I'm really not sure we should be adding anything new to _extensions.

@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

This was discussed on the CTC call today. Consensus decision was not to land this change, largely because (a) it opens up the possibility of a slippery slope with other formats that others may want to support and (b) since JSON-LD is really just JSON, there's a simple workaround.

If you look at https://www.npmjs.com/package/activitystreams-context, you can see an example of a module that loads JSON-LD using the *.json file extension. For all reasonable cases, this works well enough and using the *.jsonld file extension is not strictly required.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

Just noticed this PR.

@darobin

the idiomatic way to do it is with require('./foo.json')

Am I getting it correct that you wanted an require('file.jsonld') support out-of-the-box?
I'm pretty sure that would promote erroneous use-cases.

require() should not be used for file loading after your app is started.
You also wouldn't want to run require() on a huge amount of small files, even in a sync script.

The correct way to process a small json file is to read file first (and do that in an async way) and then do a json parse. The correct way to process a huge json file containing a lot of separate entries involves a streaming json parser.

@darobin
Copy link
Author

darobin commented Oct 28, 2015

I won't object to the decision further, but I feel that the rationale for rejecting the change is unconvincing:

  • The slippery slope argument is a well-known fallacy: if we saw a large influx of json extensions coming it would hold, but I don't think that's the case here.
  • I am well aware that you can just use .json, that's pretty obvious and has never been in doubt, but that then forces us to special-case those files in order to serve them over HTTP (as indicated in the PR).

@darobin
Copy link
Author

darobin commented Oct 28, 2015

@ChALkeR This does not promote anything that isn't already supported, the goal of the PR is simply to avoid special-casing some types of JSON and/or introducing non-idiomatic dirty workarounds (which we have to do now, and it seems will have to continue doing).

@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

@darobin ... in those cases, the only fallback would be the workaround the @Fishrock123 suggests, albeit keeping in mind that the workaround involves using an undocumented and unsupported "feature"

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

@darobin What's your exact use-case, once again?

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

Btw, one more thing to note: Module API stability index is Locked.
This PR should have been automatically rejected just because it's a semver-minor feature request in the module system.

Refs:
https://nodejs.org/api/modules.html#modules_modules
https://nodejs.org/api/documentation.html#documentation_stability_index

@darobin
Copy link
Author

darobin commented Oct 28, 2015

@jasnell I know. We don't want to go there, sticking with the unpleasant special-casing hack we have rather than switching to an unpleasant unsupported-feature hack is better :) I guess that groups with "TC", "WG", etc. in their name tend to lead to this sort of trade-off.

@ChALkeR I'm not sure what's unclear? The use case is to load JSON in the same manner that require() can do it today, but for a standard class of JSON that has an extension different .json. (I didn't label this semver-minor.)

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

@darobin Guessing that this is not theoretical, so I suppose that you have some app where you want to utilize that.

  1. Is that a server-side app? A desktop app?
  2. Do you load that file at the start-up time? At the runtime?
  3. Do you want to load a single (or a definite small amount) of hard-coded files? Or is that a directory / generated files / files supplied by user input?

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

@darobin As for the semver-minor, that label is correct, it doesn't matter who added it. Note that this PR is requesting an API change that is backward compatible, but not forward compatible, hence the semver-minor tag.

The doc clearly states:

Only fixes related to security, performance, or bug fixes will be accepted.

This is neither of those.

For this PR to properly get in (even putting all the other uncertainty aside), the stability index should be first lowered, and my guess would be that it doesn't worth that.

@darobin
Copy link
Author

darobin commented Oct 28, 2015

@ChALkeR Indeed, it's not theoretical; it wouldn't make for a very interesting theoretical case :)

It is both server and client, in fact many of the JSON-LD files we have are part of libraries that we reuse in various places. The server loads these files and uses the information in order to process things (e.g. to know that a given type found here is equivalent to another). Some client apps will load them at build time (where the lack of support for loading .jsonld is also a nuisance), others over HTTP (where the MIME type needs to be correct).

I guess "bug" is in the eye of the beholder :) Anyway, I disagree with the decision but I don't want to insist for something rather small.

@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

@darobin ... on possible approach you could take here is to do a polyfill wrap around the built-in require method. It's a bit hacky, but using require.resolve you can locate the path to the JSON-LD file and load it but fall through to the built in require if the file does not use the .jsonld path.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

@darobin I'm pretty sure that you have something wrong in your use-cases.

Note that require() is both blocking (it introduces a lag on all requests when used at runtime) and caching (the amount of used memory would grow if you do an unlimited amount of those).

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

@Fishrock123

I mean a module that sets Module._extensions['.jsonld'] = Module._extensions['.json'];.

I belive that require.extensions should be used instead of Module._extensions here (if one insists on using it, which I wouldn't recommend). require.extensions is exposed on the purpose. It's still deprecated and not recommended to use, but it's a bit better than doing the same with Module._extensions.

@rvagg
Copy link
Member

rvagg commented Oct 28, 2015

Here's your slippery slope https://en.wikipedia.org/wiki/List_of_XML_markup_languages

And yes, XML might have been a mistake to all us now enlightened folks, but we're now using JSON for basically the same things that we were using XML for and filename extension bloat is inevitable as people come up with specific variants. If we add .jsonld then the argument for rejecting the next one is more difficult than the argument for rejecting the first one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants