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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Upgrading dependencies for compatibility with Node 20 and general maintenance #926

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

Conversation

msohaill
Copy link

@msohaill msohaill commented Jan 15, 2024

List of changes:

  • Upgrading all dependencies to latest stable/compatible versions.
  • No major API adjustments required apart from mongoose MongoId being instantiated with new and mongoose no longer accepting callbacks. We used to use these queryCallbackFactory and updateCallbackFactory functions that actually ran every single query twice! I've migrated and fixed this behaviour so (technically) all queries should be twice as fast now 馃槀
  • Winston logger also needed some API updates, but nothing severe.
  • Database service no longer needs to emit db connected event as Mocha has updated their runners and Mongoose buffers queries.
  • Tests are fixed according to Fix unit tests聽#925

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

  • npm run test is successful!
  • Ran through most major workflows using the dashboard and all are successful/exhibit the same behaviour as prior.

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Questions for code reviewers?

None come to mind.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Listed change(s) in the Changelog
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged and published in downstream modules

@pierreTklein
Copy link
Member

Drive-by comment: thanks for cleaning up this repository. Do you still need a review on this? Happy to review it.

@msohaill
Copy link
Author

msohaill commented Jun 3, 2024

@pierreTklein For sure, that would be great! I think this just slipped through the cracks after the hackathon

hackerDetails,
logger.updateCallbackFactory(TAG, "hacker")
);
return logger.logUpdate(TAG, "hacker", Hacker.findOneAndUpdate(query, hackerDetails, { new: true }));
Copy link
Member

Choose a reason for hiding this comment

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

What's "new: true" here?

Copy link
Author

Choose a reason for hiding this comment

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

By default, the newer Mongoose version returns the entity as it was before applying the update. Old Mongoose version had the opposite behaviour, so to replicate we must pass { new: true } (docs).

JSON.stringify(hacker.toJSON())
chai.assert.deepStrictEqual(
res.body.data,
JSON.parse(JSON.stringify(hacker)),
Copy link
Member

Choose a reason for hiding this comment

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

Why are we parsing a stringified hacker? Could we instead just do hacker? Not a rhetorical question. I am genuinely curious.

Copy link
Author

@msohaill msohaill Jun 5, 2024

Choose a reason for hiding this comment

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

So true, I think this was the most elegant solution I could find at that point (since hacker is a complex object of complex types like ObjectId and unfortunate fields like _id so it can't be directly compared to the response data). I hadn't realized the toJSON method of the Schemas were overridden which is why the method didn't work for me. Just pushed a fix

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.

None yet

3 participants