Skip to content

refactor(bindings/node)!: Change is_exist to exists to align with nodejs API style #5731

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 11, 2025
Merged

refactor(bindings/node)!: Change is_exist to exists to align with nodejs API style #5731

merged 6 commits into from
Mar 11, 2025

Conversation

yihong0618
Copy link
Contributor

Which issue does this PR close?

this patch chore node biding also from commnet #5717

and also chore the package.json version to fix audit.

@yihong0618 yihong0618 requested a review from suyanhanx as a code owner March 10, 2025 14:37
@github-actions github-actions bot added the releases-note/chore The PR has a title that begins with "chore" or changes other small things that hard to tell label Mar 10, 2025
Xuanwo
Xuanwo reviewed Mar 10, 2025
View reviewed changes
@Xuanwo
Copy link
Member

Xuanwo commented Mar 11, 2025

Nodejs does have an exists API but deprecated: https://nodejs.org/api/fs.html#fsexistspath-callback

Maybe we can add access API in the future.

@Xuanwo Xuanwo changed the title chore: node binding is_exist to exists and fix audit chore: node binding is_exist to exists Mar 11, 2025
@Xuanwo
Copy link
Member

Xuanwo commented Mar 11, 2025

Hi @yihong0618, this update will introduce a breaking change to Node.js.

Would you be willing to update upgrade.md so we can inform users about it? It's also fine to start another PR for this.

@Xuanwo Xuanwo changed the title chore: node binding is_exist to exists refactor!(bindings/node): Change is_exist to exists to align with nodejs API style Mar 11, 2025
@Xuanwo Xuanwo changed the title refactor!(bindings/node): Change is_exist to exists to align with nodejs API style refactor(bindings/node)!: Change is_exist to exists to align with nodejs API style Mar 11, 2025
@github-actions github-actions bot added the releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" label Mar 11, 2025
@yihong0618
Copy link
Contributor Author

will do this in this PR

@Xuanwo
Copy link
Member

Xuanwo commented Mar 11, 2025

Also, this change is not a chore, it's big refactor that changed nodejs users API. I have updated the PRs title.

@Xuanwo Xuanwo removed the releases-note/chore The PR has a title that begins with "chore" or changes other small things that hard to tell label Mar 11, 2025
@yihong0618
Copy link
Contributor Author

Also, this change is not a chore, it's big refactor that changed nodejs users API. I have updated the PRs title.

learned that thanks

Xuanwo
Xuanwo reviewed Mar 11, 2025
View reviewed changes
bindings/nodejs/upgrade.md Outdated
@@ -1,3 +1,11 @@
# Upgrade to v0.53
Copy link
Member

Choose a reason for hiding this comment

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

The next version of nodejs will be 0.48:

opendal/bindings/nodejs/package.json

Line 4 in 3cfce47

"version": "0.47.11",

Xuanwo
Xuanwo approved these changes Mar 11, 2025
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @yihong0618 for this change!

@Xuanwo Xuanwo merged commit 5d63451 into apache:main Mar 11, 2025
182 of 183 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

@suyanhanx suyanhanx Awaiting requested review from suyanhanx suyanhanx is a code owner

Assignees
No one assigned
Labels
releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yihong0618 @Xuanwo