Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
new_audit: json-ld audit #8328
new_audit: json-ld audit #8328
Changes from 19 commits
47e7427
14e5d76
19e5511
e407bba
5be7b74
7da5fa5
a1470cf
39377d0
f7cb682
b7cdf44
c9c8672
e0563f9
b19564c
3d4633f
ea65f67
416d60b
cacabd2
0ff4070
1c15871
82d2785
46ad970
2d42a4c
534f10f
1243e70
6e9a8d8
41c8e28
ad58799
96cf12d
4d8198a
fc3b034
f65b807
991bf3c
87e2897
aac8717
2ac1ace
8d63bf2
055065a
7a3ad59
b578c23
53a932d
72fa128
d84fb24
9e74c1f
e51903b
0bad7c4
88a234d
8849076
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this feels like a recipe for struggles on next browserify update 😬
maybe it's time to investigate moving to something better maintained/more flexible post 5.0
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.
still a bit concerned about this but I think whatever we end up doing will be hacky in some way 🤷♂️
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.
should we add a test that the bundle output does not contain some code in
douge-json-parse
? so we remember to fix this when it break.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.
the build throws if we forget this, so it will definitely fail the tests :)
The issue is that
jsonlint-mod
's bundled js has a reference to this file, which isn't included in the module. Browserify throws when it tries to resolve it.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.
I wonder if we should just include the link to the tool here rather than force them to go to the other audit?
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.
This is the message for the manual structured data audit:
The Structured Data Linter doesn't look very welcoming, so maybe we can remove it and just link the SDTT from both audits.
Do we even need the manual audit? If we removed it we'd need to change the title of the automatic audit to not only mention JSON-LD. But maybe that's misleading, if we say your structured data is fine but we only checked JSON-LD?
@AVGP any thoughts on this?
(Related: should we call the audit
structured-data-automatic
internally or justjson-ld
?)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.
I'll defer to @AVGP for removal of the manual one.
json-ld
seems like a reasonable audit id to me. Do we foresee this growing in scope to include other types of data?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.
Renamed the audit to
json-ld
.Will bring up description/manual audit with @AVGP again in a week or two, but shouldn't be merge blocking.
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.
Updates after talking to @AVGP:
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.
Yep, I think there's value in having the manual audit as people get confused with "valid JSON-LD" vs. "Valid SD for rich results" and such, plus the SDTT catches most of that, so linking directly to it is a good idea.
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.
I'm not positive that percentage is what we care most about here, maybe we just report # invalid?
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.
Changed to "N invalid snippets"
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.
can we reuse a type for this somewhere from sd-validation types?
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.
It's kinda like
ExpandedSchemaRepresentationItem
– is there actually anything special about that type beingExpanded
?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.
I don't totally remember :) buuuuuuut I think it's just that there are more compact forms/shortcuts/references that get resolved to their actual values?
https://json-ld.org/spec/latest/json-ld-api/#expansion
You've been much deeper in this more recently than me though, so I'll leave it to your judgement. We are trying to access the
@type
andname
properties though so have some type would be niceThere 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.
I've added a type for it now. Turns out we weren't actually returning a valid
ExpandedSchemaRepresentation
fromexpand
, so fixed that type too.That's a good point, ideally we should use the expanded representation and look for "http://schema.org/name" on there. What do you think of having the the validator return an object with
errors
andexpandedSchema
properties, instead of just the errors array?Fwiw, I've never actually seen anyone do anything fancy with JSON-LD in the wild.
For reference, here are some examples of fancy JSON-LD we can use as test cases. (SDTT doesn't think the language map one is valid, but I think it's wrong.)
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.
Ah nice catch!! Yeah that sounds reasonable to me, though maybe if it's not particularly common we punt for now. Just file an issue?
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.
Filed: #8668
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.
👍