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

Added LD+JSON asset and test case #1936

Merged
merged 19 commits into from Sep 23, 2018
Merged

Conversation

bre7
Copy link
Contributor

@bre7 bre7 commented Aug 25, 2018

Closes #1935

πŸ“‹ TODO

Review ✨

πŸ‘Œ DONE

  • 🚨 Resources MUST be valid URLs, not paths.
    • Uses public-url parameter. Avoids creating a new flag but spams the DOM
  • Deal with nested values for schema attributes
  • Added missing meta attribute msapplication-config. Complements Add support for HTML meta assetsΒ #747
  • Dependencies are collected but the AST isn't saved correctly (still has old files)
  • Prevent Parcel from stripping script's type: type="application/ld+json"
  • Minify JSON (transform) Handled by htmlnano
    • JSON is minified by default when using JSON.stringify()

πŸ’ Possible Solution for URLs

Ask for URL using a new parameter (published-url ?)

  • Probably the best solution but requires adding an extra parameter to the CLI

Assume the user will enter a full URL (warn otherwise) in the schema

  1. When parsing the schema, extract the path and treat it as any local path
  2. Resolve dependency
  3. Write back the URL + collected path
  • Obvious trade-off: Parcel will assume all asset URLs in the schema target the site in development, when in fact they can be different domains.

Use public-url

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Nice work! πŸ’―

src/Parser.js Outdated
@@ -20,6 +20,7 @@ class Parser {
this.registerExtension('vue', './assets/VueAsset');
this.registerExtension('json', './assets/JSONAsset');
this.registerExtension('json5', './assets/JSONAsset');
this.registerExtension('json-ld', './assets/JSONLDAsset');
Copy link
Member

Choose a reason for hiding this comment

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

let's make the extension .jsonld as specified here: https://json-ld.org/spec/latest/json-ld/#iana-considerations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

πŸ‘

"logo",
"photo",
"image"
];
Copy link
Member

Choose a reason for hiding this comment

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

Good start. I guess we should go through the specs and find all the possible attributes that are URLs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if they list every single property, but https://schema.org/ImageObject has a dependents list: "Instances of ImageObject may appear as values for the following properties"

@bre7
Copy link
Contributor Author

bre7 commented Aug 25, 2018

@devongovett , can I add a new cli parameter ? I can't really see another way to (properly) solve the issue with URLs/paths. Or reuse public-url (will result in cluttered html) ?

@@ -247,13 +249,15 @@ class HTMLAsset extends Asset {
} else if (type === 'tag') {
if (
(rendition.type === 'js' && node.tag === 'script') ||
(rendition.type === 'css' && node.tag === 'style')
(rendition.type === 'css' && node.tag === 'style') ||
(rendition.type === 'jsonld' && node.tag === 'script')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just check isAstDirty ?

collectDependencies() {
for (let schemaKey in this.ast) {
// only check for single values, not nested data
// todo: check for nested data
Copy link
Member

Choose a reason for hiding this comment

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

make a recursive function for this?

this.ast[schemaKey] = assetPath;
this.isAstDirty = true;

if (this.options.publicURL === '/') {
Copy link
Member

Choose a reason for hiding this comment

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

so I take it JSON LD requires full URLs not paths? If so, then this is probably fine to show a warning.

Maybe this check should check whether the public URL starts with / instead of fully equaling it?

this.isAstDirty = true;

if (this.options.publicURL === '/') {
console.warn(
Copy link
Member

Choose a reason for hiding this comment

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

use the parcel logger for this

'image',
'thumbnail',
'screenshot',
'primaryImageOfPage'
Copy link
Member

Choose a reason for hiding this comment

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

You could look through https://schema.org/URL for more URL instances. Maybe Video/MediaObject and a few others of those would be good to support too.

@bre7
Copy link
Contributor Author

bre7 commented Aug 26, 2018

I still think publicURL, even if it does work, is unsuitable for this specific use case since it ends up spamming the DOM (which imo, it shouldn't)

) {
node.content = rendition.value;
}

// Delete "type" attribute, since CSS and JS are the defaults.
if (node.attrs) {
// Unless it's application/ld+json
if (node.attrs && node.attrs.type !== 'application/ld+json') {
Copy link
Contributor Author

@bre7 bre7 Aug 26, 2018

Choose a reason for hiding this comment

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

Quick fix. Proper solution: #1924

@bre7
Copy link
Contributor Author

bre7 commented Sep 4, 2018

@devongovett needs a final review

@devongovett devongovett merged commit 3ed33fd into parcel-bundler:master Sep 23, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants