Upgrade to libzim 9.4.0 #171
Conversation
kelvinhammond
changed the title
kelvinhammond
force-pushed
the
upgrade_9.4.0
branch
from
4243c5c to
94848e3
Compare
Currently 9.4.0 has not been release, this is a work in progress
I only added it for tracking purposes as I ended up not using it
kelvinhammond
force-pushed
the
upgrade_9.4.0
branch
from
94848e3 to
cfd67a1
Compare
kelson42
requested review from
benoit74 and
Copilot
and removed request for
Copilot
We don't really need a shared_ptr to Blob similar to why we don't need one for std::string, it manages it's memory internally.
Finally, went through all of the code line by line and the node-addon-api examples. Decided to go with the `TypedThreadSafeFunction` and make some wrapper classes to separate the logic. I ran `test/makeLargeZim.ts` quite a times and no crashes. openzim#80 Added some extra logging, may not be required.
kelvinhammond
requested review from
benoit74,
Copilot and
kelson42
and removed request for
benoit74 and
Copilot
There was a problem hiding this comment.
Pull request overview
This PR upgrades node-libzim to use libzim version 9.4.0, introducing new functionality and making several architectural improvements including OpenConfig support, enhanced illustration handling with IllustrationInfo, global cluster cache management, and refactored thread-safe function implementations.
Key Changes:
- Added OpenConfig class for configuring archive opening behavior
- Introduced IllustrationInfo class for more detailed illustration metadata
- Migrated cluster cache functions from archive-level to global scope
- Refactored thread-safe function wrappers to use TypedThreadSafeFunction
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/zim.test.ts | Added tests for StringProvider, IllustrationInfo, OpenConfig, and updated cluster cache tests to use global functions |
| test/makeLargeZim.ts | Updated to use modern imports and enabled custom item testing |
| src/writerItem.h | Refactored to use TypedThreadSafeFunction wrappers and added static helper methods |
| src/suggestion.h | Removed unnecessary HandleScope declarations |
| src/search.h | Removed unnecessary HandleScope declarations |
| src/openconfig.h | New file implementing OpenConfig class for archive configuration |
| src/module.cc | Added initialization for OpenConfig and IllustrationInfo, plus global cluster cache functions |
| src/item.h | Updated getDirectAccessInformation to return structured data with isValid field |
| src/index.js | Exported new classes and global cluster cache functions |
| src/illustration.h | New file implementing IllustrationInfo class for illustration metadata |
| src/entryrange.h | File deleted (not shown in diffs) |
| src/entry.h | Removed unnecessary HandleScope declaration |
| src/creator.h | Enhanced addIllustration and addMetadata to support IllustrationInfo and multiple provider types |
| src/contentProvider.h | Refactored to use TypedThreadSafeFunction with FeedTSFN wrapper and added move semantics |
| src/common.h | Added new constructor references and removed unnecessary HandleScope declarations |
| src/blob.h | Changed from shared_ptr to value semantics and added InstanceOf/GetConstructor methods |
| src/archive.h | Added OpenConfig support, enhanced illustration methods, removed archive-level cluster cache functions |
| package.json | Updated version to 4.0.0-dev0 and upgraded dependencies |
| download-libzim.js | Replaced deprecated url.parse with URL constructor |
| bundle-libzim.js | Fixed ARM64 architecture path |
| binding.gyp | Updated ARM64 library paths |
| README.md | Added local development documentation |
| .github/workflows/ci.yml | Removed macos-13 from CI matrix |
| .env | Updated LIBZIM_VERSION to 9.4.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kelvinhammond
force-pushed
the
upgrade_9.4.0
branch
from
19bf547 to
9f4cd78
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Napi::Value getSize(const Napi::CallbackInfo &info) { | ||
| if (!provider_) { | ||
| throw Napi::Error::New( | ||
| info.Env(), "StringProvider has been moved and is no longer valid."); | ||
| } |
There was a problem hiding this comment.
The error message incorrectly states that StringProvider 'has been moved', but the moved state is internal implementation detail. Consider rephrasing to 'StringProvider is in an invalid state' or 'StringProvider has already been consumed'.
| Napi::Value getSize(const Napi::CallbackInfo &info) { | ||
| if (!provider_) { | ||
| throw Napi::Error::New( | ||
| info.Env(), "FileProvider has been moved and is no longer valid."); | ||
| } |
There was a problem hiding this comment.
The error message incorrectly states that FileProvider 'has been moved', but the moved state is internal implementation detail. Consider rephrasing to 'FileProvider is in an invalid state' or 'FileProvider has already been consumed'.
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
Thank you very much, that's a good and huge PR!
Most of these code changes are well beyond my expertise ; nothing was hurting my eyes, so it is probably good to me.
I have one minor remark regarding CI, that's all for me.
I really hope the new FeedTSFN approach is going to work well, that would be a significant game changer.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
libzim 9.4.0 has been released, this is a work in progress
Diff: https://github.com/openzim/libzim/compare/9.3.0...9.4.0?diff=unified&w=#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f
Fixes Issues
test/zim.test.ts#164Todos
src/archive.h, PR: https://github.com/openzim/libzim/pull/958/filesAdd support for file descriptors since nodejs supports those.Will do in another ticket.ObjectWrap), relevant PR: https://github.com/openzim/libzim/pull/997/filesAddNvm, it's the same objectItem ItemDataDirectAccessInfoIllustrationInfosupport toCreatorExtra Todos
Trying to fix: Threading issues that have causes issues for the last few years. Try
#whatever.HandleScopethat I but in places where it's not required. They should only be used, from my understanding, when we're creating JavaScript objects on the stack / heap and we want them removed as they aren't returned. Think for loop and modifying objects that aren't returned.Napi::External(env, **a_reference**)in a few places, is this actually safe, will the reference be handled immediately and are we copying the data fully before it is removed from the stack. Basically will javascript create ourObjectWrapimmediately when we call it's constructorNapi::Functionand in that constructor are we properly copying and not keeping reference to theNapi::Externalpointer.Moving fromKept thoseAsyncWorkerto just astd::threadwhen adding ItemMay need toWasn't neededRef()from within theObjectWrap'd object so it's not garbage collected then unref when destroying the thread safe functionTypedThreadSafeFunctionreferencesstd::shared_ptrif we're not copying or passing the object around, could we just use an object. I think originally I thought we'd be passing around the values likeentrymore, verify this isn't the case.node-addon-api may already catch exception, check if it does and if these can caught by javascript. If so, remove the try catches we have all over the place.Confirmed, it does. I still added some more logging, it doesn't catch errors too well when it's an internal nodejs error :/To break out into another ticket (issue)
Add support for file descriptors since nodejs supports those.Add support for file descriptors since nodejs supports those. #172