Conversation
davidlehn
commented
Feb 23, 2020
- An event handler option seemed like a good approach for a way to allow custom handling of warnings (to display in UIs or throw errors or whatever).
- I was thinking a "strict mode" handler could be available that would turn warnings into errors.
- The event approach with chainable handlers also turned out to work well for the case of capturing and replaying events for cached processed contexts.
- I imagine other events beyond warnings might be useful.
- Open to ideas for alternate approaches.
| @@ -517,9 +526,19 @@ api.createTermDefinition = ({ | |||
| } | |||
|
|
|||
| if(reverse.match(KEYWORD_PATTERN)) { | |||
There was a problem hiding this comment.
What's an example where this would match? Looks like the above checks wouldn't allow {"@reverse": "@foo"} or similar to get this far.
There was a problem hiding this comment.
Previous checks don't specifically look at @reverse, except to verify that it's a string.
There was a problem hiding this comment.
It checks it's a string, _expandIris it, then _isAbsoluteIri checks that. Seems like a keyword like string wouldn't get beyond those steps. Is there an example where it would?
There was a problem hiding this comment.
Just created w3c/json-ld-api#389, as the order of checks was inverted.
|
Some rambling thoughts on this patch:
|
dlongley
left a comment
There was a problem hiding this comment.
Thanks for taking care of this @davidlehn!
I agree with your latest comments ... and I would also ask, any reason why the event handler stuff can't support async? We could add await code, etc. -- and it would only get called whenever event handlers were actually installed. My understanding is that we made the processing functions async so this should be possible... but I also feel like we discussed this already and you said it couldn't/shouldn't be done for some reason, but I can't recall what the issue was.
|
Some of the events happen in createTermDefinition. If event handlers were to be async, we'd have to make that async, and callers like _expandIri async, and maybe others up the stack. I was afraid that might have performance effects, but I didn't check. Another case where the in-progress benchmarking code needs to be improved so we can test such changes. I figured for version 1 the handlers could be sync and if we change to async/await in the future, they'll still work. |
- Current use is for custom handling of warnings. - Replay events when using cached contexts. - Flexible chainable handlers. Can use functions, arrays, object code/function map shortcut, or a combination of these. - Use handler for current context warnings.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
+ Coverage 92.67% 93.37% +0.70%
==========================================
Files 23 24 +1
Lines 2880 2899 +19
==========================================
+ Hits 2669 2707 +38
+ Misses 211 192 -19
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|