Add ability to load custom Experiment Engine UI #101

krithika369 · 2021-09-28T04:08:49Z

This PR adds the ability to load a remote UI component exposed by an experiment engine, at the /experiments path of the Turing app. This is made possible by the following 2 configurations (added to ui/.env.development):

REACT_APP_DEFAULT_EXPERIMENT_ENGINE
REMOTE_COMPONENTS_PROXY_PATHS

The first one introduces the concept of a 'default' experiment engine, tied to the Turing app. Although Turing support multiple experiment engines when configuring routers, the /experiments endpoint is meant to configure the experiments themselves and this will only be possible for one experiment engine that is recognised as the default. In the future, this env var will be removed and the value will instead be obtained through the Turing API.

The 2nd env var will be used only in the local environment, to proxy API requests required by the remote component so that CORS issues can be bypassed.

The remote component is created and consumed with the use of Module Federation. Some information on the main changes are described below.

Modifications

ui/package.json - Upgrade to CRA5 alpha build, use craco to override the Webpack configs
ui/craco.config.js - Add Module Federation settings
ui/src/setupProxy.js - Add proxy settings for API calls made by the remote component
ui/src/utils/remoteComponent.js, ui/src/hooks/useDynamicScript.js - Make it possible to load the remote module dynamically, as opposed to specifying the experiment engine remote in the Craco configs (ref: module-federation-examples).
ui/src/experiment/ExperimentsRouter.js - Dynamically load the experiment engine's landing page or show an appropriate message if it is not configured or if the load fails.
ui/src/App.js - Register the /experiments route.
ui/src/bootstrap.js - Moved the contents of ui/src/index.js here to let it be loaded asynchronously.

The other changes in this PR are automatic formatting fixes.

References

krithika369 · 2021-09-28T04:10:45Z

ui/package.json

+    "react-scripts": "5.0.0-next.37"
+  },
+  "resolutions": {
+    "react-scripts/**/postcss-normalize": "10.0.1"


Context: facebook/create-react-app#11278 (comment)

Currently, this will lead to a warning on yarn install:

warning Resolution field "postcss-normalize@10.0.1" is incompatible with requested version "postcss-normalize@10.0.0"

krithika369 · 2021-09-28T04:23:01Z

ui/src/setupProxy.js

@@ -1,28 +1,51 @@
-const proxy = require("http-proxy-middleware");
+const { createProxyMiddleware } = require("http-proxy-middleware");


http-proxy-middleware dependency version has been upgraded in the new CRA.

krithika369 · 2021-09-28T07:46:28Z

ui/.env.development

+# JSON string map, key being the (unroutable) path used by the remote app for API calls,
+# and value being the URL to proxy to. Used only in local development mode, to bypass CORS.
+# More info: https://create-react-app.dev/docs/proxying-api-requests-in-development/
+REMOTE_COMPONENTS_PROXY_PATHS=


Used by setupProxy.js for local development. REACT_APP_ prefix is not required here.

romanwozniak · 2021-09-29T08:05:36Z

ui/.gitignore

@@ -15,3 +15,6 @@ yarn-error.log*

 .env.development.local
 /.eslintcache
+
+# linting
+.husky/


I realise now there is a .gitignore inside this generated folder, so it's unnecessary to mention it here - removed.

romanwozniak · 2021-09-29T09:07:33Z

ui/.env.development

@@ -14,4 +14,13 @@ REACT_APP_USER_DOCS_URL=[{"href":"https://github.com/gojek/turing","label":"Turi
 # Link to the .proto file used by the Kafka Result Logger when using Protobuf serialization
 REACT_APP_RESULT_LOG_PROTO_URL=

+# Default Experiment Engine Remote UI settings
+REACT_APP_DEFAULT_EXPERIMENT_ENGINE_REMOTE_APPNAME=
+REACT_APP_DEFAULT_EXPERIMENT_ENGINE_REMOTE_URL=


Since these two variables needs to be defined simultaneously (either both or none), they can be combined into a single env variable defined as JSON object, similar to REACT_APP_USER_DOCS_URL.

ui/src/bootstrap.js

ui/src/experiment/ExperimentsRouter.js

romanwozniak · 2021-09-29T09:26:03Z

ui/src/experiment/ExperimentsRouter.js

+  }
+
+  // Load component from remote host
+  const Component = React.lazy(


can it be called ExperimentsLandingPage then?

romanwozniak · 2021-09-29T09:27:25Z

ui/src/experiment/ExperimentsRouter.js

+  );
+
+  return (
+    <React.Suspense fallback="Loading Experiments">


What's the difference between this fallback="Loading Experiments" and "Loading Experiment Exgine ..." at L:36? In what scenarios each of them will be shown?

Loading Experiment Engine happens when the remoteEntry.js file is being retrieved and appended to the html head. This part is now required because we are not setting the remote module's configurations in craco.config.js at build time anymore.

fallback="Loading Experiments" or later for the router form, may be fallback="Loading Experiment Configuration" is a placeholder while React lazy-loads the component (using loadComponent in the preceding lines). It's hardly noticeable but still required - without the fallback property, the rendering will fail in the first attempt.

Refactored the file to use <FallbackView/> for all scenarios where the remote component is not successfully loaded.

romanwozniak · 2021-09-29T09:43:21Z

ui/src/experiment/ExperimentsRouter.js

+  if (!ready || failed) {
+    return (
+      <EuiPage>
+        <EuiPageBody>
+          <EuiPageHeader>
+            <EuiPageHeaderSection>
+              <PageTitle title="Experiments" />
+            </EuiPageHeaderSection>
+          </EuiPageHeader>
+          <EuiPageContent>
+            <EuiText size="s" color="subdued">
+              {!defaultExperimentEngine.url
+                ? "No default Experiment Engine configured"
+                : !ready
+                ? "Loading Experiment Exgine ..."
+                : "Failed to load Experiment Exgine"}
+            </EuiText>
+          </EuiPageContent>
+        </EuiPageBody>
+      </EuiPage>
+    );
+  }
+
+  // Load component from remote host
+  const Component = React.lazy(
+    loadComponent(defaultExperimentEngine.appName, "./ExperimentsLandingPage")
+  );
+
+  return (
+    <React.Suspense fallback="Loading Experiments">
+      <Component projectId={projectId} />
+    </React.Suspense>
+  );


Suggested change

if (!ready || failed) {

return (

<EuiPage>

<EuiPageBody>

<EuiPageHeader>

<EuiPageHeaderSection>

<PageTitle title="Experiments" />

</EuiPageHeaderSection>

</EuiPageHeader>

<EuiPageContent>

<EuiText size="s" color="subdued">

{!defaultExperimentEngine.url

? "No default Experiment Engine configured"

: !ready

? "Loading Experiment Exgine ..."

: "Failed to load Experiment Exgine"}

</EuiText>

</EuiPageContent>

</EuiPageBody>

</EuiPage>

);

}

// Load component from remote host

const Component = React.lazy(

loadComponent(defaultExperimentEngine.appName, "./ExperimentsLandingPage")

);

return (

<React.Suspense fallback="Loading Experiments">

<Component projectId={projectId} />

</React.Suspense>

);

// Load component from remote host

const ExperimentsLandingPage = React.lazy(

loadComponent(defaultExperimentEngine.appName, "./ExperimentsLandingPage")

);

return ready

? (

<React.Suspense fallback="Loading Experiments">

<ExperimentsLandingPage projectId={projectId} />

</React.Suspense>

) : (

<EuiPage>

<EuiPageBody>

<EuiPageHeader>

<EuiPageHeaderSection>

<PageTitle title="Experiments" />

</EuiPageHeaderSection>

</EuiPageHeader>

<EuiPageContent>

<EuiText size="s" color="subdued">

{ failed

? (!!defaultExperimentEngine.url

? "Failed to load Experiment Exgine"

: "No default Experiment Engine configured"

) : "Loading Experiment Exgine ..."

}

</EuiText>

</EuiPageContent>

</EuiPageBody>

</EuiPage>

);

But more specifically, I wonder if this component should be broke down into two:

When defaultExperimentEngine.url is not configured at all then we don't need to try to load something from remote host and just render "No default Experiment Engine configured" msg

When defaultExperimentEngine.url is configured, then useDynamicScript and loadComponent are called and proper remote component is rendered.

Thanks for the suggestion. Refactored this file.

romanwozniak

Also, your changes introducing many formatting changes, which makes the review process less pleasant. We use prettier for having a shared formatting configuration (it's defined here), and formatting is triggered with husky's pre-commit hook.

Seems like you are using a newer version of prettier than I do and --jsx-bracket-same-line option is deprecated since v2.40, hence the discrepancy. Can you please replace --jsx-bracket-same-line in package.json with --bracket-same-line and reformat your branch with:

yarn prettier "src/**/*.{js,jsx,ts,tsx,json,css,scss,md}" --bracket-same-line --write

krithika369 · 2021-09-30T01:34:13Z

@romanwozniak thanks for the review. I've addressed your comments. Could you have another look?

Krithika Sundararajan added 4 commits August 17, 2021 18:19

Add sample module federation for list experiments view

8ac3a06

Include remote custom experiment engine views

47e93f0

Merge from master

fcc6449

Add dynamic loading of remote component

7618587

krithika369 commented Sep 28, 2021

View reviewed changes

Add cache busting for remote script file

03eafce

krithika369 commented Sep 28, 2021

View reviewed changes

krithika369 changed the title ~~[WIP] Add Ability to load custom Experiment Engine UI~~ Add ability to load custom Experiment Engine UI Sep 28, 2021

krithika369 requested review from romanwozniak and ashwinath September 28, 2021 12:55

romanwozniak reviewed Sep 29, 2021

View reviewed changes

Krithika Sundararajan added 3 commits September 30, 2021 08:55

Address PR comments

0e9cf2c

Bump up prettier version in deps and update the formatting flag

c0c2060

Merge from main

6c37a8a

romanwozniak approved these changes Sep 30, 2021

View reviewed changes

krithika369 merged commit d836c15 into caraml-dev:main Sep 30, 2021

This was referenced Oct 27, 2021

Relax MLP library dependency version #113

Closed

Upgrade MLP library dependency version #114

Merged

krithika369 deleted the exp_engine_module_federation branch June 14, 2022 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to load custom Experiment Engine UI #101

Add ability to load custom Experiment Engine UI #101

krithika369 commented Sep 28, 2021 •

edited

Loading

krithika369 Sep 28, 2021

krithika369 Sep 28, 2021

krithika369 Sep 28, 2021

romanwozniak Sep 29, 2021

krithika369 Sep 30, 2021

romanwozniak Sep 29, 2021

romanwozniak Sep 29, 2021

romanwozniak Sep 29, 2021

krithika369 Sep 30, 2021

romanwozniak Sep 29, 2021

romanwozniak Sep 29, 2021

krithika369 Sep 30, 2021

romanwozniak left a comment

krithika369 commented Sep 30, 2021

		@@ -1,28 +1,51 @@
		const proxy = require("http-proxy-middleware");
		const { createProxyMiddleware } = require("http-proxy-middleware");

Add ability to load custom Experiment Engine UI #101

Add ability to load custom Experiment Engine UI #101

Conversation

krithika369 commented Sep 28, 2021 • edited Loading

Modifications

References

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

romanwozniak left a comment

Choose a reason for hiding this comment

krithika369 commented Sep 30, 2021

krithika369 commented Sep 28, 2021 •

edited

Loading