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

revert arguments when call callFunctionOnService to spread variable #6450

Closed
wants to merge 1 commit into from

Conversation

deckyfx
Copy link

@deckyfx deckyfx commented Jan 31, 2024

What, How & Why?

in version 12+, the User function called from the client always sends arguments wrapped in Array,
I compare the code with version 11+ and found out that function callFunction pass the args to next function callFunctionOnService as a single array argument instead of spread arguments

This should resolve #6447

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary

In the latest version, when user call remote functions, the argument sent is wrapped inside an array,
Because when `call this.callFunctionOnService` the argument passed is `args` instead of `...args`
Copy link

cla-bot bot commented Jan 31, 2024

Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @decky-shark. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.

Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

Thanks for catching this and providing a PR! Ideally, I would love if we could also add a test to ensure this doesn't regress.

@kraenhansen kraenhansen self-assigned this Jan 31, 2024
@kraenhansen
Copy link
Member

We do seem to have tests for this, so I'll have to look into why they're not failing https://github.com/realm/realm-js/blob/main/integration-tests/tests/src/tests/sync/user.ts#L636-L672

@kraenhansen
Copy link
Member

Unfortunately, I wasn't able to pick your change into my branch, but I updated the test and implementation in #6451 and as such I'll close this PR.

You'll be rightfully attributed in the changelog and release notes. Thanks again!

@deckyfx
Copy link
Author

deckyfx commented Feb 1, 2024

np, as long as this issue is resolved, thanks for your attention <3

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User function arguments is wrapped in an Array
3 participants