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

Log refactoring #193

Merged
merged 11 commits into from
May 6, 2021
7 changes: 6 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ module.exports = {
'arrow-body-style': 'off',
'object-curly-newline': 'warn',
'semi': productionError,
'no-return-assign': 'off'
'no-return-assign': 'off',
'no-restricted-imports': ['error', {
name: 'electron-log',
importNames: ['default'],
message: 'You should import from logging/logger instead'
}]
},

overrides: [
Expand Down
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ The first time you run the app, you will need to set up the accounts you want to
Now you can set up the exporters - where the data will be sent to. The CSV exporter is enabled by default.
If you want to export to YNAB, see instructions at the end of the README.

### Report a problem

We are still in beta, and you may find errors.
Please use the **REPORT A PROBLEM** button in the app to report to us.

Use this button to find the **logs folder** as well.

Comment on lines +34 to +40
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please review

## Running in a development environment

### Prerequisites
Expand Down
2 changes: 1 addition & 1 deletion src/backend/consts.ts → src/app-globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { app, remote } from 'electron';
import { mkdirSync } from 'fs';
import path from 'path';

const App = app || remote.app;
export const App = app || remote.app;

if (process.env.NODE_ENV !== 'production') {
const localUserData = path.resolve('userData');
Expand Down
11 changes: 0 additions & 11 deletions src/backend/eventEmitters/compositeEventPublisher.ts

This file was deleted.

11 changes: 0 additions & 11 deletions src/backend/eventEmitters/consoleEmitter.ts

This file was deleted.

13 changes: 13 additions & 0 deletions src/backend/eventEmitters/loggerEmitter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { BudgetTrackingEventEmitter, EventPublisher } from '@/backend/eventEmitters/EventEmitter';

interface Logger {
info: (...params: any[]) => void
}

export function buildLoggerEmitter(logger: Logger): EventPublisher {
const loggerEmitter = new BudgetTrackingEventEmitter();
loggerEmitter.onAny((eventName, eventData) => {
logger.info(`${eventName}:`, eventData);
});
return loggerEmitter;
}
Comment on lines +1 to +13
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 not a very smart method, but it is here for a reason.

Here, in the eventEmitters folder, in the context of the eventEmitters, we know the event types, so we can parse them to string (with special care for each event), and direct them to the right level.

So, continuously, we can use this method to

  1. Print specific message depends on the Event.
  2. Print to the console or to the file, by level (because the logger argument supports both console.[level] and logger.[level]

2 changes: 1 addition & 1 deletion src/backend/import/importTransactions.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { userDataPath } from '@/app-globals';
import { EnrichedTransaction } from '@/backend/commonTypes';
import * as configManager from '@/backend/configManager/configManager';
import { userDataPath } from '@/backend/consts';
import * as bankScraper from '@/backend/import/bankScraper';
import { ScaperScrapingResult, Transaction } from '@/backend/import/bankScraper';
import * as categoryCalculation from '@/backend/import/categoryCalculationScript';
Expand Down
15 changes: 2 additions & 13 deletions src/backend/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@ import { createTransactionsInExternalVendors } from '@/backend/export/exportTran
import { scrapeFinancialAccountsAndFetchTransactions } from '@/backend/import/importTransactions';
import moment from 'moment';
import * as configManager from './configManager/configManager';
import { buildCompositeEventPublisher } from './eventEmitters/compositeEventPublisher';
import { buildConsoleEmitter } from './eventEmitters/consoleEmitter';
import * as Events from './eventEmitters/EventEmitter';
import outputVendors from './export/outputVendors';
import * as bankScraper from './import/bankScraper';
import * as Events from './eventEmitters/EventEmitter';

export { CompanyTypes } from 'israeli-bank-scrapers-core';
export { outputVendors };
Expand All @@ -16,7 +14,7 @@ export { Events };
export const { inputVendors } = bankScraper;

export async function scrapeAndUpdateOutputVendors(config: configManager.Config, optionalEventPublisher?: Events.EventPublisher) {
const eventPublisher = createEventPublisher(optionalEventPublisher);
const eventPublisher = optionalEventPublisher || new Events.BudgetTrackingEventEmitter();

const startDate = moment()
.subtract(config.scraping.numDaysBack, 'days')
Expand All @@ -35,12 +33,3 @@ export async function scrapeAndUpdateOutputVendors(config: configManager.Config,
throw e;
}
}

function createEventPublisher<Name>(optionalEventPublisher: Events.EventPublisher | undefined): Events.EventPublisher {
const eventPublishers = [buildConsoleEmitter()];
if (optionalEventPublisher) {
eventPublishers.push(optionalEventPublisher);
}
const compositeEventPublisher = buildCompositeEventPublisher(eventPublishers);
return compositeEventPublisher;
}
5 changes: 1 addition & 4 deletions src/background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { app, BrowserWindow } from 'electron';
import installExtension, { VUEJS_DEVTOOLS } from 'electron-devtools-installer';
// eslint-disable-next-line import/no-extraneous-dependencies
import { createProtocol } from 'vue-cli-plugin-electron-builder/lib';
import CreateLogger from './logging/logger';
import logger from './logging/logger';
import Sentry from './logging/sentry';
// import './store';

Expand All @@ -14,9 +14,6 @@ const isDevelopment = process.env.NODE_ENV !== 'production';
// be closed automatically when the JavaScript object is garbage collected.
let mainWindow: BrowserWindow | null;

global.logger = CreateLogger(app);
const { logger } = global;

function createWindow() {
// Create the browser window.
mainWindow = new BrowserWindow({
Expand Down
File renamed without changes.
42 changes: 20 additions & 22 deletions src/logging/logger.ts
Original file line number Diff line number Diff line change
@@ -1,29 +1,27 @@
import { App } from 'electron';
import logger from 'electron-log';
import { App } from '@/app-globals';
import logger, { LogFunctions, LogLevel } from 'electron-log'; // eslint-disable-line no-restricted-imports
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you will try to import logger from 'electron-log', you will get an ESLint error, because we want you to use our custom-defined logger.
But here we have to import the logger from electron-log to configure it.

import fs from 'fs';
import { EOL } from 'os';
import path from 'path';

declare module 'electron-log' {
interface ElectronLog {
getLastLines: (n: number) => string
}
}
export type Logger = LogFunctions
export type Levels = LogLevel

export default function CreateLogger(app: App) {
logger.info(`Welcome to ${app.getName()} log`);
logger.info(`Version: ${app.getVersion()}`);
logger.info(`Welcome to ${App.getName()} log`);
logger.info(`Version: ${App.getVersion()}`);

const onError = (error: Error) => {
logger.error(error.message ? error.message : error);
if (error.stack) logger.debug(error.stack);
};
logger.catchErrors({ onError });
const onError = (error: Error) => {
logger.error(error.message || error);
if (error.stack) logger.debug(error.stack);
};
logger.catchErrors({ onError });
Comment on lines +13 to +17
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note, this catching only the UnhandledRejection.

At least for the Vue part, we need to open a new issue to catch exceptions during the render process (I even can't say what exactly happened during such case. For in React, for example, the component tree will crash- if you're not using error boundary)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


logger.getLastLines = (n) => {
const lines = fs.readFileSync(logger.transports.file.getFile().path).toString().split(EOL);
const lastLines = lines.slice(lines.length - n);
return lastLines.join(EOL);
};
export const getLastLines = (n: number) => {
const lines = fs.readFileSync(logger.transports.file.getFile().path).toString().split(EOL);
const lastLines = lines.slice(lines.length - n);
return lastLines.join(EOL);
};

return logger;
}
export const getLogsFolder = () => path.dirname(logger.transports.file.getFile().path);

export default logger as Logger;
9 changes: 2 additions & 7 deletions src/main.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import VueCompositionAPI, { h, ref } from '@vue/composition-api';
import electron from 'electron';
import { ElectronLog } from 'electron-log';
import Vue from 'vue';
import logger from './logging/logger';
import Sentry from './logging/sentry';
import App from './ui/components/App.vue';
import SplashScreen from './ui/components/SplashScreen.vue';
import LoggerPlugin from './ui/plugins/logger';
import vuetify from './ui/plugins/vuetify';
import router from './ui/router';
import store from './ui/store';
Expand All @@ -17,10 +15,6 @@ process.on('unhandledRejection', (error) => {
console.error(error);
});

const logger: ElectronLog = electron.remote.getGlobal('logger');
logger.info('The renderer process got the logger');
Vue.use(LoggerPlugin, { logger });

Vue.use(VueCompositionAPI);

Vue.config.productionTip = process.env.NODE_ENV !== 'production';
Expand All @@ -33,6 +27,7 @@ new Vue({
name: APP_NAME,

setup(_, { root }) {
logger.info('Vue started');
const loaded = ref(false);
root.$store.restored.then(() => {
loaded.value = true;
Expand Down
2 changes: 1 addition & 1 deletion src/manual/setupHelpers.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable no-console */
import { getConfig } from '@/backend/configManager/configManager';
import { configFilePath } from '@/backend/consts';
import { configFilePath } from '@/app-globals';
import { getYnabAccountDetails } from '@/backend/export/outputVendors/ynab/ynab';
import { getFinancialAccountNumbers } from '@/backend/import/importTransactions';

Expand Down
2 changes: 1 addition & 1 deletion src/ui/components/App.vue
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
import { shell } from 'electron';
import Importers from '@/ui/components/app/Importers';
import Exporters from '@/ui/components/app/Exporters';
import { trackPage } from '@/analytics';
import { trackPage } from '@/logging/analytics';
import ReportProblemDialog from './app/ReportProblemDialog';
import MainContent from './app/MainContent';
import { repository } from '../../../package.json';
Expand Down
7 changes: 4 additions & 3 deletions src/ui/components/app/MainContent.vue
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ import { scrapeAndUpdateOutputVendors, Events } from '@/backend';
import { AccountsState, handleEvent } from '@/ui/components/app/accountsState';
import store from '@/ui/store';
import ConfigEditor from '@/ui/components/app/ConfigEditor.vue';
import { initAnalyticsEventHandling } from '@/analytics';
import { Levels } from '../shared/log/types';
import { initAnalyticsEventHandling } from '@/logging/analytics';
import logger, { Levels } from '@/logging/logger';

const statusToColor = {
NOT_STARTED: null,
Expand Down Expand Up @@ -124,7 +124,8 @@ export default defineComponent({
function initEventHandlers(eventEmitter: Events.BudgetTrackingEventEmitter, accountsState: Ref<UnwrapRef<AccountsState>>) {
eventEmitter.onAny((eventName, eventData) => {
const message = eventData?.message || eventName;
const logLevel = eventData?.error ? Levels.Error : Levels.Info;
const logLevel: Levels = eventData?.error ? 'error' : 'info';
logger[logLevel](message);
return handleEvent({ ...eventData, message, level: logLevel }, accountsState.value);
});
initAnalyticsEventHandling(eventEmitter);
Expand Down
11 changes: 8 additions & 3 deletions src/ui/components/app/ReportProblemDialog.vue
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
</v-row>
</v-container>
<small>*indicates required field</small>
<br>
<small>You can find the logs here: {{ logsDir }}</small>
Comment on lines +63 to +64
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

</v-form>
</v-card-text>
<v-card-actions>
Expand Down Expand Up @@ -95,6 +97,8 @@ import { shell } from 'electron';
import Sentry from '@/logging/sentry';
import LogSheet from '@/ui/components/shared/LogSheet';
import os from 'os';
import { getLastLines, getLogsFolder } from '@/logging/logger';
import { defineComponent } from '@vue/composition-api';
import { repository } from '../../../../package.json';

const createGithubIssueLink = (title, details, log) => {
Expand Down Expand Up @@ -130,7 +134,7 @@ const defaultFormData = {
attachLogs: true,
};

export default {
export default defineComponent({
name: 'ReportProblemDialog',
components: { LogSheet },
props: {
Expand All @@ -146,6 +150,7 @@ export default {
validateEmail: false,
formData: { ...defaultFormData },
raw: null,
logsDir: getLogsFolder()
};
},
computed: {
Expand Down Expand Up @@ -193,12 +198,12 @@ export default {
}
},
resetForm() {
this.raw = this.$logger.getLastLines(10);
this.raw = getLastLines(10);
this.formData = { ...defaultFormData };
this.validateEmail = false;
},
},
};
});
</script>

<style>
Expand Down
4 changes: 2 additions & 2 deletions src/ui/components/app/accountsState.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// eslint-disable-next-line max-classes-per-file
import { Events } from '@/backend';
import { Levels } from '@/ui/components/shared/log/types';
import { UnwrapRef } from '@vue/composition-api';
import { AccountStatus, AccountType } from '@/backend/eventEmitters/EventEmitter';
import { Levels } from '@/logging/logger';
import { UnwrapRef } from '@vue/composition-api';

export class AccountState {
id: string;
Expand Down
Empty file.
8 changes: 5 additions & 3 deletions src/ui/components/shared/LogSheet.vue
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
</template>

<script>
import { defineComponent } from '@vue/composition-api';
import { getLastLines } from '@/logging/logger';
import LogLines from './LogLines';

export default {
export default defineComponent({
name: 'LogSheet',
components: {
LogLines
Expand All @@ -44,10 +46,10 @@ export default {
textToShow() {
if (this.raw) return this.raw;

return this.$logger.getLastLines(10);
return getLastLines(10);
},
},
};
});
</script>

<style>
Expand Down
8 changes: 0 additions & 8 deletions src/ui/components/shared/log/types.ts

This file was deleted.

5 changes: 0 additions & 5 deletions src/ui/plugins/logger.ts

This file was deleted.

2 changes: 1 addition & 1 deletion src/ui/store/plugins/persisted-config.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { configManager } from '@/backend';
import { configFilePath } from '@/backend/consts';
import { configFilePath } from '@/app-globals';
import VuexPersistence from 'vuex-persist';

export default (configModuleName: string) => new VuexPersistence<any>({
Expand Down