Added LD+JSON asset and test case #1936

bre7 · 2018-08-25T17:35:50Z

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

When parsing the schema, extract the path and treat it as any local path
Resolve dependency
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

devongovett

Nice work! 💯

devongovett · 2018-08-25T17:49:30Z

src/Parser.js

@@ -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');


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

devongovett · 2018-08-25T17:50:48Z

src/assets/JSONLDAsset.js

+  "logo",
+  "photo",
+  "image"
+];


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

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 · 2018-08-25T20:56:12Z

@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) ?

bre7 · 2018-08-25T21:50:40Z

src/assets/HTMLAsset.js

@@ -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')


Why not just check isAstDirty ?

devongovett · 2018-08-26T01:10:52Z

src/assets/JSONLDAsset.js

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


make a recursive function for this?

devongovett · 2018-08-26T01:13:00Z

src/assets/JSONLDAsset.js

+        this.ast[schemaKey] = assetPath;
+        this.isAstDirty = true;
+
+        if (this.options.publicURL === '/') {


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?

devongovett · 2018-08-26T01:13:09Z

src/assets/JSONLDAsset.js

+        this.isAstDirty = true;
+
+        if (this.options.publicURL === '/') {
+          console.warn(


use the parcel logger for this

devongovett · 2018-08-26T01:16:45Z

src/assets/JSONLDAsset.js

+  'image',
+  'thumbnail',
+  'screenshot',
+  'primaryImageOfPage'


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 · 2018-08-26T02:30:17Z

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)

bre7 · 2018-08-26T02:40:53Z

src/assets/HTMLAsset.js

        ) {
          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') {


Quick fix. Proper solution: #1924

bre7 · 2018-09-04T18:13:58Z

@devongovett needs a final review

Added LD+JSON asset and test

eeddf2f

devongovett requested changes Aug 25, 2018

View reviewed changes

bre7 added 9 commits August 25, 2018 15:01

Changed ext to jsonld

810602a

Added more attributes for schema

b614c5d

Check only simple values. Added todo comment

c578ffc

Fixed test

44da9dd

yarn format

b6e4c27

Prevent Parcel from stripping script's type for JSONLD

ddd6c2b

Removed dupe attrib

301fcf9

Add deps on generated asset

d747754

Enable/Disable JSON pretty print when in dev/prod

ef1ef6f

bre7 added 3 commits August 25, 2018 18:00

Added new attribute to schema list

999fe2d

Added missing meta attrib msapplication-config to HTMLAsset

c14f601

Use public-url

9ce4134

bre7 commented Aug 25, 2018

View reviewed changes

devongovett reviewed Aug 26, 2018

View reviewed changes

Support nested objects

db13b8c

bre7 added 2 commits August 25, 2018 23:31

Improved test case

d56d2f2

Reorg & doc

1be851b

bre7 commented Aug 26, 2018

View reviewed changes

Added more attributes. Improved warning msg

f416174

DeMoorJasper approved these changes Sep 22, 2018

View reviewed changes

devongovett added 2 commits September 22, 2018 13:22

Merge branch 'master' into master

42a0b0f

Slight cleanup

9e56bd3

devongovett approved these changes Sep 23, 2018

View reviewed changes

devongovett merged commit 3ed33fd into parcel-bundler:master Sep 23, 2018

devongovett pushed a commit that referenced this pull request Oct 15, 2018

Added LD+JSON asset and test case (#1936)

c7d731d

devongovett pushed a commit that referenced this pull request Oct 15, 2018

Added LD+JSON asset and test case (#1936)

809925b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added LD+JSON asset and test case #1936

Added LD+JSON asset and test case #1936

bre7 commented Aug 25, 2018 •

edited

Loading

devongovett left a comment

devongovett Aug 25, 2018

bre7 Aug 25, 2018

devongovett Aug 25, 2018

bre7 Aug 25, 2018

bre7 commented Aug 25, 2018 •

edited

Loading

bre7 Aug 25, 2018

devongovett Aug 26, 2018

devongovett Aug 26, 2018

devongovett Aug 26, 2018

devongovett Aug 26, 2018

bre7 commented Aug 26, 2018 •

edited

Loading

bre7 Aug 26, 2018 •

edited

Loading

bre7 commented Sep 4, 2018

Added LD+JSON asset and test case #1936

Added LD+JSON asset and test case #1936

Conversation

bre7 commented Aug 25, 2018 • edited Loading

📋 TODO

👌 DONE

💁 Possible Solution for URLs

devongovett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bre7 commented Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bre7 commented Aug 26, 2018 • edited Loading

bre7 Aug 26, 2018 • edited Loading

Choose a reason for hiding this comment

bre7 commented Sep 4, 2018

bre7 commented Aug 25, 2018 •

edited

Loading

bre7 commented Aug 25, 2018 •

edited

Loading

bre7 commented Aug 26, 2018 •

edited

Loading

bre7 Aug 26, 2018 •

edited

Loading