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

Add event handler #366

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add event handler #366

wants to merge 5 commits into from

Conversation

davidlehn
Copy link
Member

  • 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)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

What's an example where this would match? Looks like the above checks wouldn't allow {"@reverse": "@foo"} or similar to get this far.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Previous checks don't specifically look at @reverse, except to verify that it's a string.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just created w3c/json-ld-api#389, as the order of checks was inverted.

@davidlehn
Copy link
Member Author

Some rambling thoughts on this patch:

  • "handleEvent" params should be renamed "eventHandler".
  • I used "code" to align with error codes. Pondering how to best handle "code", "level", and maybe "type" concepts.
  • I started adding events for all the places where expansionMap/compactionMap skip something.
    Unsure how best to go about that as their functionality starts to overlap with events. The spec doesn't say dropping unknown data is a "warning", so I'm not sure what to call it to distinguish the two flows. "debug"? Maybe levels are a bad idea and it should be more like "defaultAction": "warning"? May need to fiddle with this to avoid too much output, yet be useful for debug tools.

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.

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.

lib/context.js Outdated Show resolved Hide resolved
@davidlehn
Copy link
Member Author

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-commenter
Copy link

Codecov Report

Merging #366 (1ebcc1f) into main (b7bc6d6) will increase coverage by 0.70%.
The diff coverage is 94.44%.

@@            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     
Impacted Files Coverage Δ
lib/jsonld.js 83.17% <ø> (+0.32%) ⬆️
lib/events.js 89.28% <89.28%> (ø)
lib/context.js 94.56% <100.00%> (+3.25%) ⬆️
lib/expand.js 95.95% <100.00%> (+0.28%) ⬆️
lib/fromRdf.js 99.20% <100.00%> (+0.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7bc6d6...1ebcc1f. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants