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

feat(NODE-6539): add base napi C++ template with standard Node team tooling #28

Merged
merged 7 commits into from
Nov 14, 2024

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Nov 12, 2024

Description

What is changing?

Adds a compilable and testable C++ addon that has the same API as the old compression implementation. Additionally, all shared tooling (lint, testing, and custom test tooling) is copied over.

A GHA is set up to run tests on ubuntu to demonstrate that the changes work as expected.

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

  • Ran npm run format:js && npm run format:rs script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@baileympearson baileympearson force-pushed the NODE-6539 branch 2 times, most recently from 550817b to 2b098cb Compare November 12, 2024 23:18
@baileympearson baileympearson changed the base branch from main to NODE-6538 November 12, 2024 23:18
Base automatically changed from NODE-6538 to main November 13, 2024 17:11
@baileympearson baileympearson changed the title initial commit feat(NODE-6539): add base napi C++ template with standard Node team tooling Nov 13, 2024
@baileympearson baileympearson marked this pull request as ready for review November 13, 2024 18:06
@nbbeeken nbbeeken self-assigned this Nov 13, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 13, 2024
test/tools/chai-addons.js Outdated Show resolved Hide resolved
test/tools/mongodb_reporter.js Outdated Show resolved Hide resolved
.prettierrc.json Show resolved Hide resolved
.mocharc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.clang-format Show resolved Hide resolved
"main": "index.js",
"types": "index.d.ts",
"repository": "https://github.com/mongodb-js/zstd",
"files": [
"index.d.ts",
"index.js"
],
"dependencies": {
"bindings": "^1.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised we're adding bindings here, don't we have a ticket to stop using this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But I plan to align this package with our other native packages. When/if we get to the ticket you alluded to, we can address them all at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's unfortunate if we release with the known issues coming from that package. We can merge this, but do you think it's possible we can ship the final 2.0.0 without this? We can follow up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think it is likely that we can complete that with all the other work we need to do in the next 8 work days (our timeline for this project). I am prioritizing consistency over a full overhaul/modernization of this package.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that we need to enumerate the possible file paths of the .node file so it doesn't seem like an overhaul. Meanwhile, we're potentially introducing a regression in our major release of this package since we didn't used to use this.

We likely could adapt the code in https://github.com/mongodb-js/zstd/blob/e7e8df676e3a6948b67708938495eb876d5bf117/bindings.js to just have a few different strings and it would do the work we need.

We can revisit trying to include this in the 2.0, doesn't need to hold up this PR.

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/addon.cpp Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Nov 13, 2024
@nbbeeken nbbeeken merged commit 8c40b08 into main Nov 14, 2024
4 checks passed
@nbbeeken nbbeeken deleted the NODE-6539 branch November 14, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants