WIP: Framing #365

gkellogg · 2020-02-20T02:06:55Z

Remove new-embed tests, which are redundant and wrong.

Changed framing defaults
- embed to "@once" and warn on "@first" or "@last".
- pruneBlankNodeIdentifiers based on processingMode.
- omitGraph based on processingMode.
Top level @graph omitted if omitGraph is true.
Check for invalid values of @embed.
Changes in object embedding.
Better support for graph framing.
replaced removePreserve with cleanupPreserve and cleanupNulls
Support default values for @type when framing.
Better frame validation.

gkellogg · 2020-02-22T01:28:40Z

This gets much of framing complete. More to do, but I may have a gap in working on it, so this can be reviewed and hopefully merged). This leaves nine failing tests.

Some tests are failing with Node6, which perhaps someone with more experience in the differences can see what might be going on.

lib/expand.js

gkellogg · 2020-02-23T23:16:49Z

It looks like the Node 6 failure is due to Object.entries not being defined. There is a suggestion for a PolyFill on the MDN entry for Object.entries.

if (!Object.entries) {
  Object.entries = function( obj ){
    var ownProps = Object.keys( obj ),
        i = ownProps.length,
        resArray = new Array(i); // preallocate the Array
    while (i--)
      resArray[i] = [ownProps[i], obj[ownProps[i]]];
    
    return resArray;
  };
}

Where would be the best place to apply this?

davidlehn · 2020-02-24T00:00:53Z

Short answer is to hack it to work for now. core-js for Object.entries, and a shim for Object.fromEntries. I'll try adding it. Was trying to avoid this nonsense in jsonld.js 2.x. Once 1.1 support is mostly or all done I think we'll get radical with changes for a 3.x release. That'll include updating to core-js 3, which has Object.fromEntries, and just dropping node6 support and see if anyone notices.

dlongley

Thanks @gkellogg! This is looking good. I left some comments that are almost all (all?) related to style or ES6/7 stuff. Hopefully they can be batched together. @davidlehn -- we should look into getting linting turned on/fixed for this repo so devs don't have to deal with these nits, do you know what needs to be done there?

Regarding node 6 support ... we should drop it, node 6 is EOL. It's fine to wait on that until another PR (I noticed a FIXME about it in here).

lib/frame.js

lib/util.js

davidlehn · 2020-02-24T18:56:00Z

lib/frame.js

+          return false;
+        }
+        matchThis = true;
+      } else if(types.isObject(thisFrame)) { // XXX only framing keywords


Does this "XXX" comment need to be addressed?

It was in my code, and it could relate to some of the yet-to-be fixed issues.

davidlehn · 2020-02-24T19:08:33Z

@dlongley Re linting, we do have eslint config here, and a "lint" command, but our rules didn't check for 'brace-style'. Added a PR to consider for that:
digitalbazaar/eslint-config-digitalbazaar#31

davidlehn · 2020-02-24T19:37:23Z

lib/frame.js

          Object.keys(state.graphMap[id]).sort(), [subframe], output, '@graph');
-        state.graph = state.graphStack.pop;
+        state.graphStack.pop;


Should this be calling pop? Should it have been calling it before? Should there be a test that detects this one way or the other?

We pushed on to the stack before recursing, so need to pop it back off when done. We don't need to save that, since state.graph was set just for the call.

Before it was setting state.graph to the pop function. Now it's just doing nothing. I assume this should be pop(), but it doesn't seem to matter either way in the test suite.

No, you're right, should be pop() (Ruby-isms :) ). It doesn't matter for the tests, but it might in production.

I'll test before committing an update

Fixing and filing an issue about it.

lib/frame.js

@once

- Changed framing defaults - `embed` to "@once" and warn on "@FIRST" or "@last". - `pruneBlankNodeIdentifiers` based on processingMode. - `omitGraph` based on processingMode. - Top level `@graph` omitted if `omitGraph` is `true`.

- Changes in object embedding. - Better support for graph framing.

Replaced removePreserve with cleanupPreserve and cleanupNulls.

Co-Authored-By: Dave Longley <[email protected]>

- Static '@' keywords should be safe to use with 'in'.

gkellogg marked this pull request as ready for review February 22, 2020 01:25

gkellogg requested review from davidlehn and dlongley February 22, 2020 01:28

davidlehn added this to the JSON-LD 1.1 milestone Feb 23, 2020

davidlehn reviewed Feb 23, 2020

View reviewed changes

lib/expand.js Outdated Show resolved Hide resolved

dlongley approved these changes Feb 24, 2020

View reviewed changes

davidlehn reviewed Feb 24, 2020

View reviewed changes

lib/frame.js Outdated Show resolved Hide resolved

davidlehn force-pushed the framing branch from b7f6f81 to 9e71e84 Compare February 26, 2020 01:53

Remove new-embed tests, which are redundant and wrong.

fe2a609

davidlehn force-pushed the framing branch from 9e71e84 to bf1a9e7 Compare February 28, 2020 03:07

gkellogg and others added 14 commits February 27, 2020 22:10

Changed framing defaults and handle omitGraph.

e73171b

- Changed framing defaults - `embed` to "@once" and warn on "@FIRST" or "@last". - `pruneBlankNodeIdentifiers` based on processingMode. - `omitGraph` based on processingMode. - Top level `@graph` omitted if `omitGraph` is `true`.

Check for invalid values of @embed.

3c71a7e

Fixes for object embedding and graph framing.

bd9d497

- Changes in object embedding. - Better support for graph framing.

Replace removePreserve.

3365cf9

Replaced removePreserve with cleanupPreserve and cleanupNulls.

Support default values for @type when framing.

85a8ee8

Better frame validation.

f6fea35

Apply suggestion from @davidlehn on use of Object.fromEntries.

5b8588f

Add Object.entries polyfill.

fab9a1a

Add Object.entries/fromEntries node6 support.

7ee698c

Wildcard matching on @id and other requireAll semantics

97314ae

Fix brace style.

e56f14b

Co-Authored-By: Dave Longley <[email protected]>

Fix style.

6860bc9

Use object rest spread.

4e4743a

Co-Authored-By: Dave Longley <[email protected]>

Improve conditional.

e1217fc

davidlehn and others added 5 commits February 27, 2020 22:18

Update changelog.

9387155

Go back to @graph in frame usage.

bb4b89c

Prefer 'in' over 'hasOwnProperty' when safe.

1a37e0c

- Static '@' keywords should be safe to use with 'in'.

Update eslint dependencies.

8b6ef53

Fix typo.

077fa40

davidlehn force-pushed the framing branch from bf1a9e7 to 077fa40 Compare February 28, 2020 03:18

davidlehn approved these changes Feb 28, 2020

View reviewed changes

davidlehn merged commit 7cf2f71 into master Feb 28, 2020

davidlehn deleted the framing branch February 28, 2020 03:19

WIP: Framing #365

WIP: Framing #365

Uh oh!

Conversation

gkellogg commented Feb 20, 2020 • edited by davidlehn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkellogg commented Feb 22, 2020

Uh oh!

Uh oh!

gkellogg commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidlehn commented Feb 24, 2020

Uh oh!

dlongley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidlehn Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

gkellogg Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

davidlehn commented Feb 24, 2020

Uh oh!

davidlehn Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkellogg Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

davidlehn Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

gkellogg Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

davidlehn Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gkellogg commented Feb 20, 2020 •

edited by davidlehn

Loading

gkellogg commented Feb 23, 2020 •

edited

Loading

davidlehn Feb 24, 2020 •

edited

Loading