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

feat!: expose server sync enabled app config property #2822

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jfmcquade
Copy link
Collaborator

@jfmcquade jfmcquade commented Feb 26, 2025

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

Exposes new app config properties:

  • SERVER.sync.enabled
    • Type: boolean
    • Default: true
    • Description: when false, attempts to sync data to the server are blocked, whether triggered through a template action or the regularly scheduled syncing
  • SERVER.sync.enabled
    • Type: number
    • Default: 1000 * 60 * 5 (5 minutes)
    • Description: replaces the previous SERVER_SYNC_FREQUENCY_MS property. Sets how often the app will attempt to sync data to the server

Breaking changes

This is marked as a breaking change since it removes the previous app config property, SERVER_SYNC_FREQUENCY_MS. However, as far as I can tell, this property is not overridden for any active deployment (the default value is 5 minutes), so there shouldn't actually be any knock-ons for authors.

Follow-up

Whilst this does address the linked issue, it doesn't fully address the intended use case: as the app performs a sync on launch (specifically on init of the server service), the user data is still at risk of being overwritten, even if the launch templates disable sync. #2684 would also be required to prevent this overwrite. As that may not be straightforward to implement, we may want to consider another short-term option to handle the desired user journey.

Git Issues

Closes #2819

Screenshots/Videos

New debug template: feat_server_sync_disabled

Screenshot 2025-02-26 at 15 29 17 Screenshot 2025-02-26 at 15 29 32 Screenshot 2025-02-26 at 15 30 37

Note [SERVER] sync disabled console log when triggering a sync attempt, and contrast with behaviour on debug_sync_id template.

@github-actions github-actions bot added breaking Introduces breaking changes to how content is authored feature Work on app features/modules labels Feb 26, 2025
Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

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

I'm not quite sure if this PR is pushing in the right direction to solve the underlying issue. I feel like putting the onus on authors to enable/disable syncing for specific cases is a bit of a minefield, and one we might be better off trying to avoid entirely. Equally it seems a bit strange that a single template would have the power to put a block entirely on server syncing - I could imagine maybe a general action like server_sync: disable, but not using something that depends on rendered UI.

Given that this also doesn't fully address the initial issue, I think it might be better to look for alternate approaches. E.g. if needing to avoid syncing during profile restore action then likely best if the user service that handles restoration also handles temporarily suspending the sync service as required.

At the same time I think it also highlights how probably this server sync frequency doesn't make sense as a runtime/changing property. I've added a PR targeting this branch to make it a deployment config option instead, although appreciate that no longer closes this linked issue

#2827

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces breaking changes to how content is authored feature Work on app features/modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Allow authors to disable server syncing at a template level
3 participants