Rename multi-tab setting to synchronizeTabs #1728
Conversation
schmidt-sebastian
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
| settings.synchronizeTabs !== undefined | ||
| ? settings.synchronizeTabs | ||
| : settings.experimentalTabSynchronization, | ||
| DEFAULT_SYNCHRONIZE_TABS |
There was a problem hiding this comment.
Way to go naming the default correctly ahead of time!
There was a problem hiding this comment.
Why do you assume the API proposal wasn't based on this name? :)
| if (settings) { | ||
| if (settings.experimentalTabSynchronization !== undefined) { | ||
| log.error( | ||
| 'The `experimentalTabSynchronization` setting has been renamed to ' + |
There was a problem hiding this comment.
This is probably super nitty but I don't think we usually use backticks within log messages. I'd remove or replace with '
There was a problem hiding this comment.
I didn't like the message without any quoting, but I am fine just using single quotes.
| if (settings.experimentalTabSynchronization !== undefined) { | ||
| log.error( | ||
| 'The `experimentalTabSynchronization` setting has been renamed to ' + | ||
| '`synchronizeTabs`. In a future release, the setting will be removed ' + |
There was a problem hiding this comment.
extra space in setting will
There was a problem hiding this comment.
Ugh, I thought I fixed this before sending this out. It's fixed now.
| log.error( | ||
| 'The `experimentalTabSynchronization` setting has been renamed to ' + | ||
| '`synchronizeTabs`. In a future release, the setting will be removed ' + | ||
| 'and it is recommended that you remove it from your ' + |
There was a problem hiding this comment.
I might be overly paranoid but I worry "the setting will be removed" and "recommend that you remove it" could potentially be ambiguous and lead people to remove the setting entirely. Perhaps:
In a future release, 'experimentalTabSynchronization' will be removed, so you should update your firestore.enablePersistence() call to use 'synchronizeTabs' now.
There was a problem hiding this comment.
Sure, that works and reduces code duplication :)
|
Back to you Mike for merging! Thanls |
|
cc/ @Feiyang1 Another change for the next release... |
Note: To be merged into next major release.