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

Don't use event bus in several cases #817

Merged
merged 3 commits into from
Nov 6, 2020
Merged

Conversation

davidyuk
Copy link
Member

@davidyuk davidyuk commented Oct 1, 2020

fixes partially #660

Comment on lines +19 to +26
store.watch(
({ isBackendLive }) => isBackendLive,
async (isBackendLive) => {
const targetRouteName = isBackendLive ? 'tips' : 'maintenance';
if (router.currentRoute.name === targetRouteName) return;
await router.push({ name: targetRouteName });
},
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't want to make store and router depend on each other. Can't find a better place for this code 🤔

// eslint-disable-next-line import/no-cycle
import store from '../store';

const wrapTry = async (promise) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this method from utils to don't make the whole utils to depend on the store.

@@ -12,25 +12,21 @@ import { EventBus } from '../utils/eventBus';
export default {
mounted() {
const interval = setInterval(() => EventBus.$emit('reloadData'), 10 * 1000);
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it calls only a reloadData method of App component. I think it should be extracted to the store and called explicitly.

@@ -2,9 +2,11 @@ import Vue from 'vue';
import Vuex from 'vuex';

import mutations from './mutations';
// eslint-disable-next-line import/no-cycle
Copy link
Member Author

Choose a reason for hiding this comment

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

I propose to leave it for future refactoring. When components will depend on Backend module of the store instead of backend methods directly then we will be able to get rid of cycle dependencies.

Components shouldn't depend on the backend methods file because that file obviously depends on the store (to track backend status), but the components shouldn't depend on the store explicitly for testing/flexibility purposes.

@davidyuk davidyuk marked this pull request as ready for review October 1, 2020 09:31
Copy link
Collaborator

@thepiwo thepiwo left a comment

Choose a reason for hiding this comment

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

looks okay, as far as I understand the changes

Copy link
Collaborator

@CedrikNikita CedrikNikita left a comment

Choose a reason for hiding this comment

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

TypeError: Cannot read property 'challenge' of null
while sending comment without logged in extenstion or iframe wallet

Copy link
Collaborator

@CedrikNikita CedrikNikita left a comment

Choose a reason for hiding this comment

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

Plus, actually sent message doesn't show, they are showing afrer reloading the page.

@davidyuk davidyuk force-pushed the feature/avoid-event-bus branch from 276f81e to 9abcbdf Compare November 4, 2020 23:31
Comment on lines +135 to +140
if (!useSdkWallet) {
window.location = createDeepLinkUrl({
type: 'comment', id: tipId, text, parentId,
});
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I have missed this part initially. Bringing it back to fix:

TypeError: Cannot read property 'challenge' of null

Comment on lines +148 to +149
dispatch('reloadTip', tipId),
...parentId ? [dispatch('reloadComment', parentId)] : [],
Copy link
Member Author

Choose a reason for hiding this comment

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

Fetching of both tip and comment fixes updates when replying to a comment on a tip page.

@davidyuk davidyuk requested a review from CedrikNikita November 4, 2020 23:34
Copy link
Collaborator

@CedrikNikita CedrikNikita left a comment

Choose a reason for hiding this comment

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

Changes looks reasonable, but this is not compatible with the current production version of the wallet (you can not send a comment via deepLink with this changes).

@davidyuk
Copy link
Member Author

davidyuk commented Nov 6, 2020

Do you mean because there is not applied superhero-com/superhero-wallet#571?

@davidyuk davidyuk merged commit 444c38b into develop Nov 6, 2020
@davidyuk davidyuk deleted the feature/avoid-event-bus branch November 6, 2020 03:48
@mradkov mradkov mentioned this pull request Nov 18, 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.

5 participants