-
Notifications
You must be signed in to change notification settings - Fork 209
Update rdf-canonize, node versions, and more.
#548
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
==========================================
+ Coverage 93.63% 93.71% +0.07%
==========================================
Files 24 24
Lines 2986 2992 +6
==========================================
+ Hits 2796 2804 +8
+ Misses 190 188 -2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
This pr fixes some security issues. Would be good to update the http library too to the 4.11 version. Is there any way of helping with this or testing the beta branch with the fixes? |
|
update: I tried myself installing just the newer version of the httpclient on my project and that caused an infinite loop in some cases. This may need some investigation and just updating dependencies may not just cut it |
|
Would be great if this change could be merged! |
|
ignore my previous comment the library is safe was one of my changes that had an whoopsie |
- **BREAKING**: See the `rdf-canonize` 4.0.0 changelog for **important** changes and upgrade notes. - Update to handle different RDF/JS dataset `BlankNode` format. - Enable pass through of numerous possible `rdf-canonize` options in a `canonize()` `canonizeOptions` parameter. - Update to use `rdf-canonize` options. - The `URDNA2015` default algorithm has been changed to `RDFC-1.0` from `rdf-canon`. - Complexity control defaults `maxWorkFactor` or `maxDeepIterations` may need to be adjusted to process graphs with certain blank node constructs. - A `signal` option is available to use an `AbortSignal` to limit resource usage. - The internal digest algorithm can be changed.
- Use Node.js 22.x for most jobs. - Rename workflow extension to .yaml.
- Update development dependencies. - Update karma testing. - Remove older fixes in favor of more default behavior. - Update bundle build. - Use newer corejs version. - Build with modern browserslist defaults and no IE support. - Support for older browsers requires a custom build.
7357959 to
a131d41
Compare
- Test runtime loads test files from a web server. - Allows testing of manifests on remote web servers. - Trading off some performance to align node and browser testing. - Moves some test setup code into config data and manifest.
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
| } else if(rval['@value'] === 'false' || rval['@value'] === '0') { | ||
| rval['@value'] = false; | ||
| } else if(rval['@value'] === 'True' || rval['@value'] === 'False') { | ||
| rval['@type'] = type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should simply be an else. In other words:
- If the value is
"true"(line 353), use the nativetrue. - If the value is
"false"(line 355), use the nativefalse. - If it cannot be coerced to native types (i.e., not
true,1,false, or0), keep it as is, that is, set the type to boolean, but do not convert the value to native types.
} else {
rval['@type'] = type;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it more, you should not get '1' or '0' from RDF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it more, you should not get '1' or '0' from RDF.
Supporting those was part of why the newer test exists. What did you mean?
This should simply be an else.
Yes, I got confused. Made a test suite PR to help clarify and handle another similar case that this code would fail on.
w3c/json-ld-api#669
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's confusing but please keep the test as is, see w3c/json-ld-api#669 (comment).
Your fix is almost OK, just the plain else and it should work.
- Fix previous useNativeTypes change. - Refactor to make code easier to follow. - Performance change not tested. - Handle another case for integeres similar to that from fromRdf#t0027. - Additions proposed for API test suite.
No description provided.