w3c / respec-vc

Verifiable Credential extensions to ReSpec
Other
3 stars 5 forks source link

Add addContext function #5

Closed amiller-ims closed 1 year ago

amiller-ims commented 2 years ago

Fixes #2

Adds and exports an addContext function that authors can use to cache their own context. For example,

    <script class="remove" src="https://cdn.jsdelivr.net/gh/digitalbazaar/respec-vc@1.0.0/dist/main.js"></script>
    <script>
      const url = "https://imsglobal.github.io/openbadges-specification/context.json";
      fetch(url)
      .then(res => res.json())
      .then(context => window.respecVc.addContext(url, context));
    </script>
davidlehn commented 2 years ago

Two related issues here.

I'm not sure what the sequencing of this will be. In the vc-data-model spec the createVcExamples call happens in a postProcess hook. If the "Retrieve obContext" part here is async, will the postProcess hook be run too early? Seems like a race condition in that case? A static context would work of course.

Setting things up in this way is just showing that we need to do work on making documentLoaders easier to use. The right way to do this is probably that you would append your own caching async documentLoader (probably from a helper library for such things). It could then load your custom contexts in whatever way is needed. So createVcExamples would just be using the existing machinery to load static or dynamic contexts on demand.

The extendContextLoader style used here, along with a local mapping lookup, and then adding API calls into that... is kind of odd. But maybe best to just use this patch until that can all be fixed in a more global way.

amiller-ims commented 2 years ago

The extendContextLoader style used here, along with a local mapping lookup, and then adding API calls into that... is kind of odd. But maybe best to just use this patch until that can all be fixed in a more global way.

I agree. When Manu announced respec-vc I sensed a desire to release the simplest possible thing that works. That's the approach I took here. It does put the onus on implementers to avoid race conditions and handle errors.

amiller-ims commented 2 years ago

Updated the example to show fetching the context.

davidlehn commented 2 years ago

@amiller-ims Please remove package-lock.json from that last commit.

davidlehn commented 2 years ago

Did you mean to revert the whole commit? Also I meant use rebase, or other means, to squash package-lock out of the commit history. (otherwise it stays around forever)

amiller-ims commented 2 years ago

I did mean to revert the whole commit. The change to index.html was only for my testing. I was rushing and did not think it through.

Is it OK that the commit (and revert) are in the history? I have only ever used rebase and squash when teaching myself git. I'm afraid I would make things worse if I fumbled through them now.

davidlehn commented 2 years ago

@amiller-ims It's nice to have clean commit histories in PRs when merging. Otherwise unneeded commits end up in history forever. In this case, I think you could just drop that merge commit and the one that added and reverted things? It's a good skill to learn. I use git rebase -i ... all the time. And also it's only 6 lines in the PR, so not hard to fix if mistakes are made. ;-) The alternative is we can squash when merging if needed, but it's easy to forget to do that.

amiller-ims commented 2 years ago

I'll give it a try. Thank you for the encouragement.

amiller-ims commented 2 years ago

@davidlehn I think it worked and I learned something. Thank you!