-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix theme dev session #927
Conversation
We detected some changes at packages/*/src and there are no updates in the .changeset. |
Coverage report
Show files with reduced coverage 🔻
Test suite run success872 tests passing in 441 suites. Report generated by 🧪jest coverage report action from e912857 |
@isaacroldan did this end up working? |
Yes and no, it refreshes the tokens and starts a new ruby process. But on windows it fails to terminate old processes, so you end up with multiple theme dev servers running at the same time. |
options?.signal?.addEventListener('abort', () => { | ||
commandProcess.kill('SIGTERM', {forceKillAfterTimeout: 1000}) | ||
const pid = commandProcess.pid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, after this fix, do we still need to treat windows differently here? It might be the case that now we can just kill the tree correctly with the previous code on windows as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to remove the windowsHide
property?
WHY are these changes introduced?
theme dev
needs a valid session at all times, so we need to take care of refreshing the tokens before they expire.A bug was introduced where the 90min interval was causing
theme dev
to just stop instead of properly refresh the session.Fixes #781
WHAT is this pull request doing?
setInterval
function, as it doesn't play well with awaits (but we don't need them)How to test your changes?
theme dev
for at least 2h, the session should be refreshed automatically and the server restarted.Measuring impact
How do we know this change was effective? Please choose one:
Checklist
dev
ordeploy
have been reflected in the internal flowchart.