Luigi core config refactorings #187

jesusreal · 2018-11-02T09:20:45Z

Description

Changes proposed in this pull request:

Create class for Luigi Config
Avoid using interval to check for Luigi config existency -> Luigi initializes up to 50ms faster
Update unit tests
Use correct key for screenshots folder in Cypress config

Related issue(s)

parostatkiem-zz

I like the changes!
Just a few how to be modern suggestions from my side, feel free to implement them or not.
One obsolete console.log(), 2 useless (I think) variable declarations. Please explain or remove them :)

core/src/services/config.js

parostatkiem-zz · 2018-11-05T09:18:40Z

core/src/services/config.js

+    };
+    this.config;
+
+    this.configReadyCallback = function() {};


I think here's the same story as with this.config; above. It doesn't change anything if you remove this line.

This initial value is meant to enable calling this.configReadyCallback without the need of an if block to prove if it is defined or not.

parostatkiem-zz · 2018-11-05T09:31:17Z

core/src/services/config.js

-    value = value.apply(this, parameters);
-    if (isPromise(value)) {
-      return value;
+  getConfig() {


You can be more modern here and use a getter.

get Config() { return this.config; }

and then
this.getConfig() -> this.Config.

I'm not requesting this change but I'd like you to know it :)

I consider that option, but I see unnecessary to create a breaking change for this tiny improvement.

core/test/config.spec.js

…-refactorings # Conflicts: # core/src/navigation/LeftNav.html # core/src/navigation/TopNav.html # core/src/navigation/services/navigation.js # core/src/services/routing.js

dariadomagala-sap · 2018-11-08T15:02:31Z

core/src/services/config.js

-  );
-};
+// Expose it window for user app to call Luigi.setConfig()
+window.Luigi = LuigiInstance;


Do we need to add everything to window.Luigi Wouldn't setConfig be enough?

For consistency I decided to expose the whole Luigi config instance, as this is what we are exporting through es module. But on the other side, I am in favour of exposing as less as possible to the window object, so I will apply the change you proposed. Thanks!

Now I know why I did not do that. Luigi config is a class, so setConfig is interacting with other members of the class. Therefore IMHO the best solution is to expose the whole instance

dariadomagala-sap · 2018-11-08T15:08:39Z

core/src/utilities/async-helpers.js

+      return value;
+    }
+  }
+  return wrapAsPromise(value);


Do we need this function? Wouldn't that be easier to wrap it here? :) I't not used anywhere else

Good catch, removed

dariadomagala-sap · 2018-11-09T08:03:18Z

core/src/services/config.js

-    nextValue = nextValue[propertyPath[propIndex++]];
+class LuigiConfigManager {
+  constructor() {
+    this.configReadyTimeout = {


Do we need this object?
The valueMs is not used anywhere. My suggestion would be to change this configReadyTimeout from an object to a string with just the id value.
The other option would be to use this valueMs in the timeout below, but then I don't like the name of the object, because what is an id of a timeout then?

Id of a timeout makes sense to me, it is the way you identify a timeout you set in case you want to do something with it, like removing it. I will use valueMs instead the magic number '2000' when setting the timeout and keep the object.

dariadomagala-sap · 2018-11-09T13:46:34Z

docs/navigation-configuration.md

@@ -286,7 +286,7 @@ When loading, the **viewUrl** uses the following dynamic URL parameters:
 - `sort = asc`

 ```
-Luigi.setConfig({
+window.Luigi.setConfig({


It still should work with just Luigi.setConfig right?

yes, everywhere is used with Luigi.setConfig. I just added it here to the documentation for consistency

So it's ok with this task, but I guess for the future we should set it to just Luigi.setConfig, it just seems easier ;)

dariadomagala-sap · 2018-11-09T13:56:10Z

docs/navigation-configuration.md

@@ -286,7 +286,7 @@ When loading, the **viewUrl** uses the following dynamic URL parameters:
 - `sort = asc`

 ```
-Luigi.setConfig({
+window.Luigi.setConfig({


So it's ok with this task, but I guess for the future we should set it to just Luigi.setConfig, it just seems easier ;)

…-refactorings # Conflicts: # core/src/App.html # core/src/Authorization.html

…-refactorings # Conflicts: # core/src/navigation/TopNav.html

* Refactor Luigi config code in core to be a class * Encapsulate configNotReadyCallback in luigi core config class * move object generic methods to helpers files * Use proper key in cypress config for screenshots folder * Cosmetic changes based on pull request review * Small change in docu for consistency * Remove wrapAsPromise function and use it as inline code * use timeout value in ms instead of magic number in luigi config file * Small change in docu for consistency

jesusreal added 4 commits October 31, 2018 17:33

Refactor Luigi config code in core to be a class

38bf0e9

Encapsulate configNotReadyCallback in luigi core config class

f3fb853

move object generic methods to helpers files

4aabd1a

Use proper key in cypress config for screenshots folder

18c6c98

jesusreal added enhancement New feature or request area/luigi labels Nov 2, 2018

jesusreal requested review from antiheld, dariadomagala-sap, hardl, kwiatekus, maxmarkus, parostatkiem-zz, pekura and y-kkamil as code owners November 2, 2018 09:20

Merge branch 'master' into luigi-core-config-refactorings

5262bde

parostatkiem-zz self-assigned this Nov 5, 2018

parostatkiem-zz suggested changes Nov 5, 2018

View reviewed changes

jesusreal added 2 commits November 5, 2018 11:41

Cosmetic changes based on pull request review

61ef892

Merge branch 'master' into luigi-core-config-refactorings

4cb53d4

parostatkiem-zz previously approved these changes Nov 7, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into luigi-core-config…

7add5cb

…-refactorings # Conflicts: # core/src/navigation/LeftNav.html # core/src/navigation/TopNav.html # core/src/navigation/services/navigation.js # core/src/services/routing.js

kwiatekus self-assigned this Nov 8, 2018

dariadomagala-sap reviewed Nov 9, 2018

View reviewed changes

kwiatekus previously approved these changes Nov 9, 2018

View reviewed changes

jesusreal added 3 commits November 9, 2018 10:45

Small change in docu for consistency

89b927a

Remove wrapAsPromise function and use it as inline code

31bf8e0

use timeout value in ms instead of magic number in luigi config file

5779e96

jesusreal dismissed stale reviews from kwiatekus and parostatkiem-zz via 5779e96 November 9, 2018 09:50

jesusreal requested a review from bszwarc as a code owner November 9, 2018 09:50

jesusreal requested review from kazydek, klaudiagrz and tomekpapiernik as code owners November 9, 2018 09:50

jesusreal assigned dariadomagala-sap Nov 9, 2018

parostatkiem-zz previously approved these changes Nov 9, 2018

View reviewed changes

dariadomagala-sap reviewed Nov 9, 2018

View reviewed changes

dariadomagala-sap previously approved these changes Nov 9, 2018

View reviewed changes

Small change in docu for consistency

1b59811

jesusreal dismissed stale reviews from dariadomagala-sap and parostatkiem-zz via 1b59811 November 9, 2018 13:59

Merge remote-tracking branch 'upstream/master' into luigi-core-config…

04b38ff

…-refactorings # Conflicts: # core/src/App.html # core/src/Authorization.html

kwiatekus approved these changes Nov 9, 2018

View reviewed changes

dariadomagala-sap approved these changes Nov 9, 2018

View reviewed changes

bszwarc approved these changes Nov 9, 2018

View reviewed changes

parostatkiem-zz approved these changes Nov 9, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into luigi-core-config…

6f923d8

…-refactorings # Conflicts: # core/src/navigation/TopNav.html

jesusreal merged commit 273f607 into SAP:master Nov 13, 2018

jesusreal deleted the luigi-core-config-refactorings branch December 21, 2018 08:05

misappi unassigned parostatkiem-zz and dariadomagala-sap May 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Luigi core config refactorings #187

Luigi core config refactorings #187

jesusreal commented Nov 2, 2018

parostatkiem-zz left a comment

parostatkiem-zz Nov 5, 2018

jesusreal Nov 5, 2018 •

edited

Loading

parostatkiem-zz Nov 5, 2018

jesusreal Nov 5, 2018

dariadomagala-sap Nov 8, 2018

jesusreal Nov 9, 2018

jesusreal Nov 9, 2018

dariadomagala-sap Nov 8, 2018

jesusreal Nov 9, 2018

dariadomagala-sap Nov 9, 2018

jesusreal Nov 9, 2018

dariadomagala-sap Nov 9, 2018

jesusreal Nov 9, 2018

dariadomagala-sap Nov 9, 2018

dariadomagala-sap Nov 9, 2018

Luigi core config refactorings #187

Luigi core config refactorings #187

Conversation

jesusreal commented Nov 2, 2018

parostatkiem-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusreal Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusreal Nov 5, 2018 •

edited

Loading