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

pass logger to filter for time consuming information #3296

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jobo322
Copy link
Member

@jobo322 jobo322 commented Nov 23, 2024

No description provided.

Copy link

cloudflare-workers-and-pages bot commented Nov 23, 2024

Deploying nmrium with  Cloudflare Pages  Cloudflare Pages

Latest commit: f2f6772
Status: ✅  Deploy successful!
Preview URL: https://65de4cd2.nmrium.pages.dev
Branch Preview URL: https://pass-log-for-time-consuming.nmrium.pages.dev

View logs

@jobo322 jobo322 force-pushed the pass-log-for-time-consuming-filter-22-11-1023 branch from 176128e to 9a5936a Compare November 23, 2024 03:36
@jobo322 jobo322 force-pushed the pass-log-for-time-consuming-filter-22-11-1023 branch from 98fe26b to f2f6772 Compare November 25, 2024 21:19
@@ -176,6 +178,7 @@ function KeysListenerTracker(props: KeysListenerTrackerProps) {
type: 'DELETE_EXCLUSION_ZONE',
payload: {
zone,
logger,
},
Copy link
Member

Choose a reason for hiding this comment

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

We need to ensure the reducer function remains pure. Passing the logger to the reducer introduces a side effect, which we should avoid. This issue has been consistently repeated across all the PRs you submitted

Copy link
Member

Choose a reason for hiding this comment

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

we should lift the function up that requires the logger as a parameter to the callback function and pass its result down to the reducer

Copy link
Member

Choose a reason for hiding this comment

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

We have a reducer that for example apply apodization on a spectrum. Apodization being done by nmr-processing.

We would like that nmr-processing can log some information (time spend in each filter) in the logger.

Problem is that the logger is changing another part of the state and this is not expected to be done in a reducer.

@targos Do you have some suggestions how to solve this issue ?

Copy link
Member

Choose a reason for hiding this comment

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

Hamed said it all. You need to do the processing outside of the reducer and send the result to the reducer. A reducer is not supposed to do heavy work. Just take some data and update the state with it.

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.

4 participants