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

Adding option to keywordize documents for Firestore #34

Closed
wants to merge 1 commit into from

Conversation

shen-tian
Copy link
Contributor

@shen-tian shen-tian commented Feb 13, 2019

This refers to #31 and deals with the keywordizing issue as expected.

I'd like some input on how to handle these configurable behaviour issues. In this case, we don't want to change the existing behaviour as to not break backwards compatibility. So:

  1. Is a named param provided to init the right way to go?
  2. Where should this be stored? The goto stateful place is core/firebase-state. Is that fine?

I'd also like to work on a PR for #28 that would require a similar flag. So some direction would be appreciated. Once I've got some input, I'll clean this up + update docs etc.

@deg
Copy link
Owner

deg commented Feb 25, 2019

I'm not sure about this one. I'm torn between not breaking existing users and not complicating the interface with additional flags.

Maybe a compromise would be:

In any event, I'd like the flag for this feature to be something more verbose than :fs-kw.

Basically, I want to improve the quality of the interface for new clients, while making it as easy possible for old clients to upgrade.

@shen-tian
Copy link
Contributor Author

Think the usual thing to do is to keep adding flags for altered behaviour, and then change defaults with a new major release? In that release, we can then allow previous default behivours to be turned on as you suggest.

I don't think we should really couple this with #28.

So, would :firestore-keywordize-keys be a good flag name? If so, I suggest we change to that, update docs and wrap up this feature.

@shen-tian shen-tian closed this Sep 2, 2020
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.

2 participants