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

Support JSON literals. #325

Merged
merged 5 commits into from
Sep 10, 2019
Merged

Support JSON literals. #325

merged 5 commits into from
Sep 10, 2019

Conversation

davidlehn
Copy link
Member

work-in-progress.

Copy link
Collaborator

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

@davidlehn I did some work on expansion use cases, compaction and everything else seems to work okay already. See what you think.

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Approving but left some comments for tidying a few things up.

lib/expand.js Outdated Show resolved Hide resolved
lib/fromRdf.js Outdated Show resolved Hide resolved
lib/json-canonicalize.js Outdated Show resolved Hide resolved
@davidlehn
Copy link
Member Author

I'm not sure what the guarantees of that json-canonicalize code are. It depends on the native JSON.stringify. Do we need some runtime test (maybe from the jcs browser tests) to know if the native code works as expected? The concern is if this code runs on ye olde browser or odd runtime and the JSON.stringify output differs.

@dlongley
Copy link
Member

dlongley commented Sep 4, 2019

@davidlehn,

It depends on the native JSON.stringify. Do we need some runtime test (maybe from the jcs browser tests) to know if the native code works as expected?

Sounds like we should add some canonicalization tests over here for that: https://github.com/json-ld/normalization

@gkellogg
Copy link
Collaborator

gkellogg commented Sep 4, 2019

@davidlehn,

It depends on the native JSON.stringify. Do we need some runtime test (maybe from the jcs browser tests) to know if the native code works as expected?

Sounds like we should add some canonicalization tests over here for that: https://github.com/json-ld/normalization

The code is right out of the RFC example. There's also an NPM Package referenced from the spec to implement this. If you use a package which is tested, I'm not sure what the merits of re-testing are.

@davidlehn
Copy link
Member Author

@gkellogg I may be misunderstanding, but I think the algorithm and code depends on an ES6 runtime? I thought there were some issues if you use older browsers where some behavior was not yet standardized. Am I mistaken on that point? I'm just wondering how/if we can detect that so we don't output garbage by mistake if someone runs the code on an old platform.

Also I forgot that code was in the RFC. Should use that vs the code from the repo. I also didn't realize that package existed. NPM search failed me. Maybe we should just use that.

@davidlehn
Copy link
Member Author

From the RFC "JCS relies on serialization of JSON primitives compatible with ECMAScript (aka JavaScript) beginning with version 6". How do we check, support, or error out on pre-ES6 serializers?

@davidlehn
Copy link
Member Author

I'm not sure what the best way to detect that the native serializer does what it's supposed to do. The limited JCS tests appear to run on Node.js 4-12, IE11, and recent Edge, Firefox, Safari, and Chromium. So we'll call that good enough for now.

@davidlehn davidlehn merged commit a304e04 into master Sep 10, 2019
@davidlehn davidlehn deleted the rdf-json branch September 10, 2019 17:27
@davidlehn davidlehn changed the title Support literal JSON. Support JSON literals. Sep 10, 2019
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.

3 participants