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

Vueify restart, move js files to vue, add state-switch & more #5159

Merged
merged 30 commits into from
Oct 6, 2018

Conversation

OmgImAlexis
Copy link
Collaborator

@OmgImAlexis OmgImAlexis commented Sep 8, 2018

Moved js files to vue

  • config/index
  • home/restart
  • manage/failed-downloads
  • manage/init
  • manage/index
  • manage/subtitle-missed
  • mass-update

Misc changes

  • displayShow renamed to show
  • Added state-switch component to replace loading, yes and no images.
  • Vueifed restart page
  • Replaced is_alive jsonp route with json
  • Enabled typescript validation in src directory.

</div>
<div id="restart_fail_message" style="display: none;">
<div id="restart_fail_message" v-if="status === 'failed'">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is never called on the old setup so should we just remove it?

The old js is below.

let currentPid = $('.messages').attr('current-pid');
const defaultPage = $('.messages').attr('default-page');
const checkIsAlive = setInterval(() => {
    // @TODO: Move to API
    $.get('home/is_alive/', data => {
        if (data.msg.toLowerCase() === 'nope') {
            // If it's still initializing then just wait and try again
            $('#restart_message').show();
        } else if (currentPid === '' || data.msg === currentPid) {
            $('#shut_down_loading').hide();
            $('#shut_down_success').show();
            currentPid = data.msg;
        } else {
            clearInterval(checkIsAlive);
            $('#restart_loading').hide();
            $('#restart_success').show();
            $('#refresh_message').show();
            setTimeout(() => {
                window.location = defaultPage + '/';
            }, 5000);
        }
    }, 'jsonp');
}, 100);

Copy link
Contributor

@sharkykh sharkykh Sep 9, 2018

Choose a reason for hiding this comment

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

This is never called on the old setup so should we just remove it?

On my attempt of fully vueifying this page (one of the "backlogged PRs" I told you about 😅),
I actually decided to implement this by giving the restart a 60 second timeout. This serves 2 good points:

  1. When the timeout is reached, you (can) stop trying to poll /home/is_alive. Freeing up the browser if the tab stayed open.
  2. Actually giving the user a correct status for the restart.

It's your call if you want to implement it in this PR, but either way I would suggest not removing it since it's actually something we should be doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay let's keep it, what if instead of polling we used the websocket disconnection mutation to know it's gone down? That'll happen automatically when the server disconnects and I'm pretty sure it's close to the end of the shutdown cycle where it happens.

We can then have a watch on a Vuex state socket.connected(I think?) and have the page redirect to the default page once it's reconnected.

We could also have a modal in the background the listens to these events on all pages but the restart and when it disconnects the modal blocks the page until it reconnects?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the websocket is an option too, however you'd still need to know if the app is fully initialized. Currently this is the check with the process ID - if the app is not ready no PID is sent back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could wait for an "ok" message on the socket?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the client browser isn't connected it might not receive the message,
I don't think it has the same reliability.

@OmgImAlexis OmgImAlexis added the Needs testing Requires testing to make sure it's working as intended label Sep 8, 2018
@OmgImAlexis OmgImAlexis requested a review from sharkykh September 8, 2018 14:15
@sharkykh

This comment has been minimized.

@sharkykh

This comment has been minimized.

@OmgImAlexis

This comment has been minimized.

@sharkykh

This comment has been minimized.

@OmgImAlexis

This comment has been minimized.

Signed-off-by: Alexis Tyler <[email protected]>
@OmgImAlexis OmgImAlexis changed the title move js files to .vue components and rebuild themes misc changes Sep 9, 2018
Signed-off-by: Alexis Tyler <[email protected]>
Copy link
Contributor

@sharkykh sharkykh 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 removing loglevel in #5185

name: $(this).val()
}
}).then(response => {
log.info(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace log (loglevel) with console.

log.info(response);
window.location.reload();
}).catch(error => {
log.error(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace log (loglevel) with console.

@sharkykh
Copy link
Contributor

sharkykh commented Sep 14, 2018

@sharkykh sharkykh changed the title misc changes Vueify restart, move js files to vue, add state-switch & more Sep 14, 2018
@p0psicles
Copy link
Contributor

Whats the status off this pr?

@OmgImAlexis
Copy link
Collaborator Author

@p0psicles I think it just needs a rebase and the issues @sharkykh mentioned could be seen to but that could also be moved to a new issue since I'm busy and don't want this to get stale.

@sharkykh sharkykh removed the Changelog Requires a changelog entry label Oct 5, 2018
sharkykh
sharkykh previously approved these changes Oct 5, 2018
@medariox medariox merged commit 10d9fa0 into develop Oct 6, 2018
@medariox medariox deleted the misc-changes branch October 6, 2018 09:17
@OmgImAlexis OmgImAlexis mentioned this pull request Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concluded Enhancement Needs review Needs testing Requires testing to make sure it's working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants