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

Avoid race condition in LoginPage component #49

Merged
merged 4 commits into from
Apr 24, 2020

Conversation

reiallenramos
Copy link
Contributor

@reiallenramos reiallenramos commented Apr 23, 2020

Closes #48

Changes:

  • set a flag to decide whether to display the Login snackbar

Screenshots:
When refreshing the User page:

  • if the reroute from /login to /user finishes sooner than the request to get LOGIN_MESSAGE resolves, the snackbar is never shown.
    race_condition_fixed

liangyuanruo and others added 3 commits April 10, 2020 02:15
Release 1.13.2 - Landing page tweaks, SVG QR codes for IE11
Release 1.13.3 - Documentation and bug fixes
Release 1.13.4 - Send Sentry source-maps on travis build step
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

As stated here, #48 (comment), the implementation is injected server side because we prefer to be able to update the displayed message without code changes.

That said, we are happy to accept a PR that resolves the race condition brought up in the issue.

@liangyuanruo liangyuanruo changed the base branch from master to develop April 23, 2020 16:14
@liangyuanruo liangyuanruo changed the base branch from develop to master April 23, 2020 16:16
@reiallenramos
Copy link
Contributor Author

@liangyuanruo makes sense. I'm just trying to wrap my head around your comment. Does this mean if you change an environment variable in production, you wont have to re-deploy to reflect the changes? Express picks it up instantly? 🤔

@reiallenramos reiallenramos changed the title Move LOGIN_MESSAGE from server to client (i18n) Avoid race condition in LoginPage component Apr 24, 2020
@liangyuanruo
Copy link
Contributor

@liangyuanruo makes sense. I'm just trying to wrap my head around your comment. Does this mean if you change an environment variable in production, you wont have to re-deploy to reflect the changes? Express picks it up instantly? 🤔

No, it means that changing the message is considered a system configuration, which does not require a code commit.

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

Thanks, code looks good to me. Final thing - could you switch the pull request target to develop? That is our development mainline, and we will perform a merge into master for our next release cycle.

@reiallenramos reiallenramos changed the base branch from master to develop April 24, 2020 06:04
@liangyuanruo liangyuanruo merged commit f82c2b0 into opengovsg:develop Apr 24, 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.

Login snackbar persists through the User page
3 participants