-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
JSON Language #370
JSON Language #370
Conversation
Added JSON support to prism.
Alternatively, 'variable' could be used instead of 'property', which would look nicer in my opinion but is semantically incorrect. |
See my comments on #369, they remain valid. |
Right, sorry I didn't know you could do that. Also I'm still debating on wether I should use 'property' or 'variable' for the properties. It might be a good idea to just use property and then add some css for |
Suggested by owner in #369
proper JSONP support.
Neither is
Or define it as alias (@LeaVerou did we agree on using For the implementation: I would probably go with extending javascript, too. At least for not having to duplicate a lot of code. If there are issues with using |
There is actually one extreme edge case, as described in this post, in which the JavaScript rules wouldn’t highlight JSON properly, while this implementation does. This can be fixed by extending JavaScript instead, obviously, but that seems a bit backwards IMO to include so much (C-like and JavaScript) for so few syntax features. |
If you do want me to move the JSON to a javascript extension, you'd have to tell me how to remove the things it inherits that it doesn't need. But I completely agree with @uranusjr on this one. I just checked and JSON would only inherit 2 definitions (number and boolean), which really doesn't seem to make it worthwhile. |
By the way, is there a way I can safely rename my branch, because I get a build warning email with every commit because the branch is still named gh-pages. |
That’s a known issue that will eventually be solved once we separate master and gh-pages… That would also satisfy the bower crowd. The problem is that currently, I'm not sure how to make such a split in a DRY manner. |
delete Prism.languages.foo;
You’re right, it’s not worth it just for two tokens, one of which is rather simple. @apfelbox what do you think? Also, IMO we should merge JSON and JSONP, either by having both on the same file, or in the same language definition too (any opinions?) |
Same file would make sense, but I would recommend keeping the distinction between the two. Awaiting your decision before making additional changes. |
I would argue that loading too much doesn't harm a lot. The complete javascript markup is < 1kB, compile time in the browser is practically non-existant. The only difference between JS and JSON seems to be (according to the link @uranusjr posted) native support for UTF-8 in all strings - which we could easily fix by just allowing them in JS too (again: Prism is a forgiving highlighter, no merciless linter). But: |
(as a single language definition |
Excellent point. This should be in all bold on the contributor readme I think :p I think the main issue between sharing strings in these two defs is that JSON doesn't allow single quoted strings (which also makes its regex easier, as no backreference is needed) |
Since we are being so forgiving I really can't deny that putting the jsonp in the json definition and just making it an alias is a bad idea :) |
JSONP alias added to JSON.
'null': /\bnull\b/gi, | ||
}; | ||
|
||
Prism.languages.jsonp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prism.languages.jsonp = Prism.languages.json;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow im stoopid :p
Rename a branch? I hope it's easy |
What happened here? Why did this stall? |
@chris-martin: This component has not made it into Prism yet. I think you'd better use the JS component to highlight your JSON, for now, as it seems to work properly: |
@CupOfTea696 Reversing the order of the number and string tokens shall fix this. Please refer to the JS language definition for the optimal order. |
That should've fixed it. |
I'm confused about how this can make a difference. Aren't JS objects unordered? |
In theory they are. In practice all engines follow the order in which the properties were defined. And Prism relies on this behaviour. |
(See also discussion in #441) |
This works rather well. The only thing that I see missing is that if you use the Here is a temporary work around: Prism.hooks.add('before-highlight', function(env) {
var pre = env.element.parentNode;
if (!pre || !/pre/i.test(pre.nodeName)) return;
if (/^json/i.test(env.language)) {
pre.setAttribute('data-language', 'JSON');
}
}); |
This is done automatically via the gulp build task: https://github.com/PrismJS/prism/blob/gh-pages/gulpfile.js#L52-L89 |
Is there any hope of this landing soonish? Highlighting with JS isn't very helpful as pretty much everything ends up being pretty much the same colour :) Also, I've been thinking of making it possible to style keys starting with |
@darobin I'll try to make a proper review of this PR by the end of the week. |
Hate to be that guy (I understand this is an OSS project), but any updates on a code review? This PR has been around for over a year now. |
Anything new to this? |
inside: { | ||
punctuation: /\(/ | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON doesn't have functions…
Sorry this has taken so long. I just took a look and added a few comments. |
@LeaVerou no worries! Applied the necessary changes. |
Merged, thanks! |
Hi, good work! Is this going to be released anytime soon? |
@drodil Any merged PR is immediately available to the live site. Btw, @CupOfTea696, I noticed you made this enabled by default. While I did merge it to include it in Prism, I do not agree with it being enabled by default. Could you or someone else please send another PR to remove |
I've removed the default option. |
Thank you Andrea!! |
@LeaVerou That's probably because I copied it over from javascript. Sorry about that. Probably wasn't sure on what it did and just left it that way :p |
Added JSON support to prism.