refactor(RHICOMPL-1105): contextual useFeature hook #827

vkrizan · 2020-11-13T18:25:52Z

useFeature hook now uses global EnabledFeaturesContext.

This improves:

parsing URL only once
setting and checking stored values only once
globally accessible context through useFeature hook

Also fixes:

warnings on non-boolean value passed from a stored value
reportsTableView constant (typo)

akofink

Nice refactoring! It continues to work as expected, and the warnings have disappeared. I'll defer to @bastilian for a more detailed code review, but the new context seems really nice :)

akofink · 2020-11-13T18:50:04Z

src/Utilities/EnabledFeatures.js

+    }
+};
+
+export const EnabledFeatures = ({ children }) => {


vkrizan · 2020-11-16T10:00:13Z

src/Utilities/EnabledFeatures.js

+    useEffect(() => {
+        let setFeatures = {};
+
+        const urlParams = new URLSearchParams(search || '');


There is one difference with the previous code, and that is redirection. In this context-based version there is no redirection (the passed parameters will stay in location/URL until navigation). We can add it if you think it is needed.

The redirect was/is intended.

There are other URL dependent features on the platform that we should avoid to interfere with.

Sure, I can add it.

Currently (in CI) any URL query string does a redirect. I guess we already broke that interfere promise. 😉

Currently (in CI) any URL query string does a redirect. I guess we already broke that interfere promise. 😉
Write

I'm not sure if that is actually a bug, but I'd need to (re)investigate that. I believe this is correct behaviour of react-router. If no component renders it will redirect. I think. For now there don't seem to be issues with that.

It redirects, because the original useFeature does not check available features, sets anything to local storage, and then redirects back (clearing query string). react-router does not impact it.

I believe that query string isn't used by react-router in our app.

bastilian · 2020-11-16T10:01:50Z

src/Utilities/EnabledFeatures.js

+};
+
+export const EnabledFeatures = ({ children }) => {
+    const [enabledFeatures, setEnabledFeatures] = useState(getStoredFeatures);


We might not need to keep this in a state. It would only give us a benefit if we'd have a way to set the feature flags other than via the URL.

Partially true, but a state does not hurt. We have to realize that the useEffect (defined down) can happen asynchronously, hence the state.

Further enhancements are also possible, like live feature views.

We will never need this.

bastilian · 2020-11-16T10:05:19Z

src/Utilities/EnabledFeatures.js

+
+const getStoredFeatures = () => {
+    let savedState = { ...features };
+    Object.keys(features).forEach((feature) => {


Prefer .map & spread over .forEach & X[Y] = Z

Can you please write an example?

This function could for example read like this:

const getStoredFeatures = () => { const featuresEnabled = Object.assign(...Object.keys(features).map((feature) => ( { [feature]: !!localStorage.getItem(`${LOCAL_STORE_FEATURE_PREFIX}:${feature}`) || false } )).filter((v) => !!v)); return { ...features, ...featuresEnabled }; };

Sorry, but I don't find it more readable.

Using forEach and assigning properties in an object is an obsolete way of doing this.

We aim for JavaScript written towards ES6(+), and therefore should make use of .map (and other functions) and utilise spreading.

If you look around other places in this codebase as well as other projects in the Insights organisation you'll see that it is a common (and encouraged) pattern.

bastilian · 2020-11-16T11:53:19Z

src/Utilities/EnabledFeatures.js

+    const [enabledFeatures, setEnabledFeatures] = useState(getStoredFeatures);
+    const { search } = useLocation();
+
+    useEffect(() => {


This seems to just reimplement what was the useFeature hook.

Why not try to preserve most of the original hook and make it "context aware" and simply invoke it here?

The implementation move from the hook here is the whole point of this. The hook would have to access a context an would have to be able to write to it. The problem of the hook was that each time it was used, the same was parsed over and over again. There was no global state, which this new context adds.

The implementation should be in a hook.

The point of using hooks is to keep Component bodies small and make them less complex.

useFeature hook now uses global EnabledFeaturesContext. This improves: * parsing URL only once * setting and checking stored values only once * globally accessible context through useFeature hook

codecov-io · 2020-11-16T12:52:33Z

Codecov Report

Merging #827 (5ecb65a) into master (689f458) will decrease coverage by 0.35%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
- Coverage   77.65%   77.30%   -0.36%     
==========================================
  Files          65       66       +1     
  Lines         971      978       +7     
  Branches      138      139       +1     
==========================================
+ Hits          754      756       +2     
- Misses        186      188       +2     
- Partials       31       34       +3

Impacted Files	Coverage Δ
src/constants.js	`55.00% <ø> (ø)`
src/Utilities/EnabledFeatures.js	`51.61% <51.61%> (ø)`
src/Utilities/hooks/useFeature.js	`100.00% <100.00%> (+37.03%)`	⬆️

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 689f458...5ecb65a. Read the comment docs.

vkrizan · 2020-11-16T12:59:01Z

config/setupTests.js

+
+})();
+
+global.localStorageMock = localStorageMock;


@bastilian Is this the right way to do global mocks?

You can also only mock it in the tests themselves using jest.mock.

Setting up a global mock shouldn't be needed, since only one set of tests interacts with it.

I'm not sure if jest allows to mock globals, as per the issue jestjs/jest#2098 (comment)

There is also a package to help with the localStorage (which I've skipped) https://www.npmjs.com/package/jest-localstorage-mock

In test you should not mock a global, you'd mock useFeature and for the tests of hook related code useLocation.

vkrizan · 2020-11-16T16:59:43Z

Converted to draft, after discussion with @bastilian. There are concerns about the implementation with a global context, that might possibly impact next deployment.

akofink · 2021-03-11T19:29:38Z

@vkrizan @bastilian it sounds like there were some fundamental disagreements on this one. Should we continue to try and get it merged or close it?

bastilian · 2021-03-12T11:45:02Z

@akofink The main objective of this was to do parsing of URL as well as only setting and checking stored values only once, this has meanwhile been addressed when correcting the hook to comply with the linter[1].

[1] 7a46666

akofink · 2021-03-12T15:10:03Z

Ok! So we can close this one? @vkrizan?

vkrizan requested a review from bastilian November 13, 2020 18:25

akofink approved these changes Nov 13, 2020

View reviewed changes

src/Utilities/EnabledFeatures.js

}

};

export const EnabledFeatures = ({ children }) => {

Copy link

Contributor

akofink Nov 13, 2020

Choose a reason for hiding this comment

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

💯

vkrizan commented Nov 16, 2020

View reviewed changes

bastilian reviewed Nov 16, 2020

View reviewed changes

vkrizan added 3 commits November 16, 2020 13:45

fix(RHICOMPL-1105): useFeature to return boolen when set

f8a3640

refactor(RHICOMPL-1105): contextual useFeature hook

b11e4d8

useFeature hook now uses global EnabledFeaturesContext. This improves: * parsing URL only once * setting and checking stored values only once * globally accessible context through useFeature hook

fix(RHICOMPL-1105): reportsTableView feature constant

5ecb65a

vkrizan force-pushed the improve-use-feature-1105 branch from 3151662 to 5ecb65a Compare November 16, 2020 12:45

vkrizan commented Nov 16, 2020

View reviewed changes

vkrizan marked this pull request as draft November 16, 2020 16:39

vkrizan closed this Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(RHICOMPL-1105): contextual useFeature hook #827

refactor(RHICOMPL-1105): contextual useFeature hook #827

vkrizan commented Nov 13, 2020

akofink left a comment

akofink Nov 13, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

bastilian Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

bastilian Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

codecov-io commented Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

vkrizan Nov 16, 2020

bastilian Nov 16, 2020

vkrizan commented Nov 16, 2020

akofink commented Mar 11, 2021

bastilian commented Mar 12, 2021 •

edited

Loading

akofink commented Mar 12, 2021


		})();

		global.localStorageMock = localStorageMock;

refactor(RHICOMPL-1105): contextual useFeature hook #827

refactor(RHICOMPL-1105): contextual useFeature hook #827

Conversation

vkrizan commented Nov 13, 2020

akofink left a 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

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

codecov-io commented Nov 16, 2020

Codecov Report

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

vkrizan commented Nov 16, 2020

akofink commented Mar 11, 2021

bastilian commented Mar 12, 2021 • edited Loading

akofink commented Mar 12, 2021

bastilian commented Mar 12, 2021 •

edited

Loading