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

Update mongo libs from 3.6 to 5 #8314

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

Update mongo libs from 3.6 to 5 #8314

wants to merge 5 commits into from

Conversation

bewest
Copy link
Member

@bewest bewest commented Nov 26, 2024

Revert a revert to get original pull request back.
This reverts commit 2e65b1d. Try to bring back #8026.

@bewest bewest changed the title Revert "Revert "Merge pull request #8026 from nightscout/less_frequen… Update mongo libs from 3.6 to 5 Nov 27, 2024
@bewest
Copy link
Member Author

bewest commented Nov 27, 2024

@MilosKozak Does AAPS send multiple batch items through this interface? If so, it looks like the websocket stuff may still need more work.

@bewest
Copy link
Member Author

bewest commented Nov 27, 2024

There is a list of methods targeted here:
https://github.com/mongodb/node-mongodb-native/blob/HEAD/etc/notes/CHANGES_5.0.0.md

Here's an especially relevant section:

Collection.insert, Collection.update, and Collection.remove removed

Three legacy operation helpers on the collection class have been removed:

Removed API API to migrate to
insert(document) insertOne(document)
insert(arrayOfDocuments) insertMany(arrayOfDocuments)
update(filter) updateMany(filter)
remove(filter) deleteMany(filter)

The insert method accepted an array of documents for multi-document inserts and a single document for single-document inserts. insertOne should now be used for single-document inserts and insertMany should be used for multi-document inserts.

I've completed a review of the source tree and and have converted all insert calls into insertOne. If AAPS or something else uses this to modify multiple activities, foods, profiles, or calls dbAdd via websocket with multiple documents, there will probably be issues. Also it's not clear what users are doing with the results from websocket events given to the callback.

Also, for whatever reason, remove(filter) seems to still work...
Tests have been added for areas of the api that were missing.

We need some actual testing with AAPS rigs (different/older versions would be nice as well), multiple looping systems, etc.

Another significant thing to note is that there is now a 15 second delay between entering data and seeing it reappear on the display, likely due to debouncing. We can adjust this based on feedback.

@@ -405,20 +406,22 @@ function init (env, ctx, server) {
}
// if not found create new record
console.log(LOG_DEDUP + 'Adding new record');
ctx.store.collection(collection).insert(data.data, function insertResult (err, doc) {
ctx.store.collection(collection).insertOne(data.data, function insertResult (err, ops) {
Copy link
Member Author

Choose a reason for hiding this comment

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

AAPS folks: do multiple documents come through this interface as an array? Can you provide any examples?

if (err != null && err.message) {
console.log('treatments data insertion error: ', err.message);
return;
}

var doc = data.data;

Choose a reason for hiding this comment

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

Do you have a roadmap or anything around browser support? How much longer will the codebase use var instead of let/const?

Looks like any android/ios device since 2011 should support let/const - there are no vendor-currently-supported os/browser combinations that don't support them.

Maybe the approach could be "use nightscout 15 for old browsers" and do any work on the f/e to support newer testing/frameworks in a nightscout v16 that will only support newer browsers? I think you'll rapidly paint yourself into a corner if "we're not waiting" becomes "we support your ancient devices forever"

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

Successfully merging this pull request may close these issues.

2 participants