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

TS definition and API doc for setSyncLogger #2260

Merged
merged 2 commits into from
Mar 1, 2019

Conversation

kneth
Copy link
Contributor

@kneth kneth commented Feb 28, 2019

This closes #2125 and https://github.com/realm/realm-js-private/issues/517

☑️ ToDos

  • 📝 Changelog entry
  • [ ] 📝 Compatibility label is updated or copied from previous entry
  • [ ] 🚦 Tests
  • [ ] 📝 Public documentation PR created or is not necessary
  • [ ] 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • [ ] Chrome debug API is updated if API is available on React Native

@kraenhansen
Copy link
Member

kraenhansen commented Feb 28, 2019

Because the API has not been published or documented yet we should be able to easily change its name. It's only used a handful of places in our own code. I would suggest that we simplify the naming before it goes out as a documented API:

Realm.Sync.setLogger()

Alternatively I think we should keep a list of breaking changes that we want to do for a future major version of Realm JS and I would add this renaming to that list.

@nirinchev
Copy link
Member

This doesn't need to be a breaking change. We can have a setSyncLogger and setLogger that do the same thing.

@kraenhansen
Copy link
Member

kraenhansen commented Mar 1, 2019

This doesn't need to be a breaking change. We can have a setSyncLogger and setLogger that do the same thing.

That's also a possibility, but it would be a breaking change to first publish types for setSyncLogger and remove them again when renaming. And it's probably not the best idea to have two functions with slightly different names and same behaviour. We could leave an undocumented setSyncLogger (with a deprecation warning at runtime) which calls a documented setLogger - that would be a fine solution for me.

I'm afraid to be bikeshedding here. We could just merge this, but if we want to follow a process of implementing a fast solution and documenting it when we learn from using it, we should be willing to take the hit of adapting the fast solution to the stuff we're learned along the way. If not - we should just document and provide types for functions like this, right away.

@cmelchior
Copy link
Contributor

I agree with @kraenhansen ... having the method named setLogger just feels more natural and easier. It also means we can extend it in the future with logs from pure JS (which would be nice when debugging).

There is no reason to pollute the API with methods we intend to remove anyway, especially if they haven't been made public yet.

@nirinchev
Copy link
Member

We can't remove the method, but we can leave it undocumented and document just the new one.

@kneth
Copy link
Contributor Author

kneth commented Mar 1, 2019

Required to be released together with https://github.com/realm/realm-js-private/pull/518

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.

Great!

@kneth kneth requested a review from nirinchev March 1, 2019 11:41
@kneth kneth merged commit 4218b4a into master Mar 1, 2019
@kneth kneth deleted the kneth/add-sync-logger-to-ts branch March 1, 2019 12:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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.

Document Realm.Sync.setSyncLogger
6 participants