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

Change UI configuration to a Javascript file #677

Conversation

th3M1ke
Copy link
Contributor

@th3M1ke th3M1ke commented Jan 13, 2021

Which problem is this PR solving?

Short description of the changes

@th3M1ke
Copy link
Contributor Author

th3M1ke commented Jan 13, 2021

@yurishkuro I remember, that we discuss memoization of the getConfig() function here, but I found one test, which checks for depreciation each time we invoke getConfig(). Is this relevant? What is the use case for this?

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>
@th3M1ke th3M1ke force-pushed the Change-UI-configuration-to-a-Javascript-file branch from 0327864 to 2b9dadb Compare January 13, 2021 16:38
@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #677 (2b9dadb) into master (551db4c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #677   +/-   ##
=======================================
  Coverage   94.23%   94.23%           
=======================================
  Files         228      228           
  Lines        5930     5930           
  Branches     1448     1448           
=======================================
  Hits         5588     5588           
  Misses        308      308           
  Partials       34       34           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 551db4c...2b9dadb. Read the comment docs.

@yurishkuro
Copy link
Member

but I found one test, which checks for depreciation each time we invoke getConfig(). Is this relevant? What is the use case for this?

Don't know what the use case would be, given that at present the config is static as far as the UI is concerned. Maybe it was envisioning loading (and reloading) the config from the server dynamically? Where is the test?

@yurishkuro yurishkuro merged commit 1c9a9ba into jaegertracing:master Jan 14, 2021
@th3M1ke
Copy link
Contributor Author

th3M1ke commented Jan 14, 2021

Test 'processes deprecations every time getConfig is invoked' located 'packages/jaeger-ui/src/utils/config/get-config.test.js:113'

@yurishkuro
Copy link
Member

yurishkuro commented Jan 14, 2021

So from what I understand, there were already some historical config format changes such that certain properties have been renamed or moved to different location, and that's what process-deprecation.ts module is fixing. In terms of memoization, the deprecation logic should run at the same time as the UIConfig() function is being called, i.e. the combined result should be memoized. I think the calls to getConfig() are the ones that should be memoized.

function getJaegerUiConfig() {
if(typeof window.UIConfig === 'function') {
return UIConfig();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the result be memorized? I see a dozen log lines with the following config:

function UIConfig() {
    console.log("loading config");
}

Copy link
Member

Choose a reason for hiding this comment

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

@th3M1ke
Copy link
Contributor Author

th3M1ke commented Jan 15, 2021

@yurishkuro I suggest move the discussion about memoization here #679

vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
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.

Change UI configuration to a Javascript file
3 participants