Revert "feat: Support runtime configuration"

[asadazam93]

Reverts #377

The discussions MFE is broken on stage. My assumption is that this PR is the culprit

[codecov]

Codecov Report

Base: 85.04% // Head: 85.06% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (67b0b33) compared to base (da108a2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
+ Coverage   85.04%   85.06%   +0.02%     
==========================================
  Files         134      134              
  Lines        2741     2746       +5     
  Branches      762      762              
==========================================
+ Hits         2331     2336       +5     
  Misses        388      388              
  Partials       22       22              

Impacted Files	Coverage Δ
src/components/NavigationBar/data/api.js	30.00% <100.00%> (ø)
src/data/api.js	100.00% <100.00%> (ø)
src/data/constants.js	100.00% <100.00%> (ø)
src/discussions/cohorts/data/api.js	100.00% <100.00%> (ø)
src/discussions/comments/data/api.js	92.00% <100.00%> (+0.33%)	⬆️
src/discussions/data/api.js	100.00% <100.00%> (ø)
src/discussions/learners/data/api.js	100.00% <100.00%> (ø)
src/discussions/posts/data/api.js	91.89% <100.00%> (+0.22%)	⬆️
src/discussions/topics/data/api.js	85.71% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[arbrandes]

@asadazam93, can you help me understand what is causing the issue?

[asadazam93]

@arbrandes I am not sure either. I think the issue might be with something else. Still under investigation

[asadazam93]

@arbrandes This PR did turn out to be the culprit. LMS_BASE_URL was being returned as null which caused the issue. Do you have any idea how to fix this?

[asadazam93]

And I was not able to reproduce this issue locally. So maybe there is an environment variable that we need to add in production?

[arbrandes]

@asadazam93, the only environment variable related to the change is LMS_BASE_URL. And ultimately, the code path that retrieves it was not changed: it would still be getConfig().LMS_BASE_URL. The calls would just be wrapped in utility functions as opposed to being saved in variables.

This is not something exclusive to this MFE. All the ones in Olive do it, so it shouldn't be controversial. There could be a typo somewhere, but one would hope unit tests would catch them. Plus, as you say, this is not reproduceable locally.

Can you give me more details on the environment where the issue is happening? Are you able to provide instructions on how to reproduce the issue? There's very little I can do if I can't see the problem happening.

[asadazam93]

@arbrandes I have tried investigating and I was not able to reproduce this locally and there are no useful logs for the error. The issue only occurs on the stage environment. It is successfully deployed but the MFE completely breaks and shows the following error in console
Minified React error #130; visit https://reactjs.org/docs/error-decoder.html?invariant=130&args[]=undefined&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings.
Let me know if there is anything else I can provide

[arbrandes]

@asadazam93, thanks for the additional info.

Correct me if I'm wrong, but pending further information there is only one way to reproduce the error, then: to redeploy the PR to stage. I'm going to re-submit the original PR so we can try do do a more thorough code review - there could be a simple logic error somewhere - but my hunch is that this could be triggering a React bug, or even a bug somewhere in the MFE other than in the lines changed.

A couple of questions in the mean time, if you don't mind:

1. Is it possible to deploy this code to stage without merging it?
2. If not, can we merge this code again and stop the pipeline so it doesn't go into production automatically?
3. Is there any way to disable minification on stage temporarily?
4. If none of the above are possible, would it be possible instead to create a replica of stage where the issue is reproducible, just without minification?
5. In any of these cases, how much visibility into the issue can you provide me for debugging purposes? An account on stage and enrollment in the course(s) that trigger the issue? A security-obscured listing of the MFE's environment variables?

Thank you for your help so far!

[asadazam93]

@arbrandes I am not sure about the root cause either. To answer your questions.

1. It is not possible to deploy without merging.
2. Yes we can merge the PR and it will be deployed on stage. We can test on stage, deployment to production is triggered manually so that is not a problem
3. This is not possible
4. You can create an account on stage and enroll in any course and debug. There is nothing useful in the logs though for these types of errors.

Note: I had to revert the PR because there were a lot of changes that needed to be deployed to production and were blocked because of this issue.

[arbrandes]

I have some tentative good news: I think I may have reproduced the underlying issue, just on another MFE and with another variable (frontend-app-course-authoring and LOGO_URL, for future reference). There is also nothing in the code to indicate why the variable would be undefined, but yet, it is.

In any case, just the fact it's happening and is reproducible gets us half way there... if that is indeed an instance of the issue we've been discussing. I'll let you know what I find.

[arbrandes]

False alarm. Turns out the other issue was completely unrelated. I opened a new PR so we can continue the conversation there.