chore: make environment adaptor globally available #855

josephaxisa · 2021-10-13T14:35:48Z

This eliminates the need for the envAdaptor context and paves the way for envAdaptor usage in sagas

- eliminates the need for the envAdaptor context - Paves the way for envAdaptor usage in sagas

github-actions · 2021-10-13T14:40:42Z

APIX Tests

0 files -     1 0 suites - 80 0s ⏱️ - 2m 28s
0 tests - 320 0 ✔️ - 307 0 💤 - 13 0 ❌ ±0
0 runs - 336 0 ✔️ - 323 0 💤 - 13 0 ❌ ±0

Results for commit 6a1973d. ± Comparison against base commit 474ee93.

This pull request removes 320 tests.

 DocTitle renders a heading with title prop content ‑  DocTitle renders a heading with title prop content
 MethodBadge renders with the provided verb ‑  MethodBadge renders with the provided verb
ApiSpecSelector it fires a SELECT_SPEC action when another spec is selected ‑ ApiSpecSelector it fires a SELECT_SPEC action when another spec is selected
ApiSpecSelector it lists all available specs ‑ ApiSpecSelector it lists all available specs
ApiSpecSelector the default spec is selected by default ‑ ApiSpecSelector the default spec is selected by default
CodeCopy displays code and clipboard UI ‑ CodeCopy displays code and clipboard UI
CodeDisplay it highlights text matching search pattern ‑ CodeDisplay it highlights text matching search pattern
CodeDisplay it syntax highlights ‑ CodeDisplay it syntax highlights
CodeDisplay utils it can syntax highlight all supported sdk langs ‑ CodeDisplay utils it can syntax highlight all supported sdk langs
CodeDisplay utils it highlights as markup for unsupported/undefined language ‑ CodeDisplay utils it highlights as markup for unsupported/undefined language
…

ensures registration happens after mounting, during the initialization phase, and not on every rerender

josephaxisa · 2021-10-13T15:03:50Z

packages/api-explorer/src/ApiExplorer.tsx

+    registerEnvAdaptor(envAdaptor)
+    setInitializing(false)
+
+    return () => unregisterEnvAdaptor()


I think this is only needed in the dev portal because in the standalone mode unmounting means a reload happened. May also be applicable in extensions. In any case, better to have it than not to. @bryans99

Better to have in my opinion if only to future proof

github-actions · 2021-10-13T15:08:09Z

APIX Tests

    1 files ±0   80 suites ±0 2m 24s ⏱️ -4s
320 tests ±0 307 ✔️ ±0 13 💤 ±0 0 ❌ ±0
336 runs ±0 323 ✔️ ±0 13 💤 ±0 0 ❌ ±0

Results for commit 4374d97. ± Comparison against base commit 474ee93.

bryans99 · 2021-10-13T15:14:11Z

packages/api-explorer/src/ApiExplorer.tsx

+    registerEnvAdaptor(envAdaptor)
+    setInitializing(false)
+
+    return () => unregisterEnvAdaptor()


Better to have in my opinion if only to future proof

bryans99 · 2021-10-13T15:20:52Z

packages/api-explorer/src/ApiExplorer.tsx

@@ -71,10 +72,10 @@ const ApiExplorer: FC<ApiExplorerProps> = ({
  declarationsLodeUrl = `${apixFilesHost}/declarationsIndex.json`,
  headless = false,
 }) => {
+  const [initializing, setInitializing] = useState(true)


Ideally this should be the redux.

I would expect an initializeExplorer action (perhaps loadExplorer instead), Consider sending the env as the property of the action (okay cos not storing in the reducer) and have a saga call the register. Have an unloadExplorer called when the component is unloaded.

The initialize/loadExplorer action gives us an opportunity to load the versions and initial spec as well.

Good point. The PR following this addresses this.

packages/api-explorer/src/ApiExplorer.tsx

packages/api-explorer/src/components/DocMarkdown/DocMarkdown.tsx

packages/api-explorer/src/components/Header/Header.spec.tsx

packages/api-explorer/src/components/SelectorContainer/SdkLanguageSelector.tsx

packages/api-explorer/src/test-utils/redux.tsx

github-actions · 2021-10-13T16:40:36Z

APIX Tests

    1 files ±0   80 suites ±0 2m 31s ⏱️ +3s
320 tests ±0 307 ✔️ ±0 13 💤 ±0 0 ❌ ±0
336 runs ±0 323 ✔️ ±0 13 💤 ±0 0 ❌ ±0

Results for commit 235c05b. ± Comparison against base commit 474ee93.

jkaster · 2021-10-13T18:49:23Z

Is Bryn's feedback going to be address in a different PR, or will this PR be updated for it?

josephaxisa · 2021-10-13T19:01:10Z

I addressed and resolved all comments already except for 1 which is part of a follow up PR.

The "initialization" status will be part of the next PR. All other feedback has been addressed.

jkaster

Looks good for this PR

bryans99

LGTM aside from comment around Loader. That can be fixed in the future

bryans99 · 2021-10-13T19:29:03Z

packages/api-explorer/src/ApiExplorer.tsx

      >
-        <ErrorBoundary logError={envAdaptor.logError.bind(envAdaptor)}>
-          <EnvAdaptorContext.Provider value={{ envAdaptor }}>
+        {initializing ? (


Sorry to jerk you around but I just realized that Loader instantiates its own version of component provider. What you had before was okay.

Having said that, I think there is opportunity to eliminate the need for ComponentsProvider in Loader at some point in the future so leave this as is.

Once redux in place, I think we can eliminate the need for the Loader in the StandaloneApiExplorer and ExtensionApiExplorer and push the logic down to here.

Thanks for looking Bryn. I agree, let's leave the change in. I'll eliminate the use of Loader from the standalone and extension top level components once it's possible.

make env adaptor globally available

6a1973d

- eliminates the need for the envAdaptor context - Paves the way for envAdaptor usage in sagas

google-cla bot added the cla: yes label Oct 13, 2021

move env adaptor registration into useEffect

4374d97

ensures registration happens after mounting, during the initialization phase, and not on every rerender

josephaxisa requested review from bryans99 and jkaster October 13, 2021 15:01

josephaxisa commented Oct 13, 2021

View reviewed changes

bryans99 previously requested changes Oct 13, 2021

View reviewed changes

feedback

bb57b9e

josephaxisa requested a review from bryans99 October 13, 2021 16:29

fix typo

235c05b

jkaster approved these changes Oct 13, 2021

View reviewed changes

bryans99 approved these changes Oct 13, 2021

View reviewed changes

josephaxisa merged commit 9a9d9fe into main Oct 14, 2021

josephaxisa deleted the jax/global-env-adaptor branch October 14, 2021 09:04

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: make environment adaptor globally available #855

chore: make environment adaptor globally available #855

josephaxisa commented Oct 13, 2021

github-actions bot commented Oct 13, 2021

josephaxisa Oct 13, 2021

bryans99 Oct 13, 2021

github-actions bot commented Oct 13, 2021

bryans99 Oct 13, 2021

bryans99 Oct 13, 2021

josephaxisa Oct 13, 2021 •

edited

Loading

github-actions bot commented Oct 13, 2021

jkaster commented Oct 13, 2021

josephaxisa commented Oct 13, 2021

jkaster left a comment

bryans99 left a comment

bryans99 Oct 13, 2021

josephaxisa Oct 14, 2021

This comment has been minimized.

chore: make environment adaptor globally available #855

chore: make environment adaptor globally available #855

Conversation

josephaxisa commented Oct 13, 2021

github-actions bot commented Oct 13, 2021

APIX Tests

josephaxisa Oct 13, 2021

Choose a reason for hiding this comment

bryans99 Oct 13, 2021

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2021

APIX Tests

bryans99 Oct 13, 2021

Choose a reason for hiding this comment

bryans99 Oct 13, 2021

Choose a reason for hiding this comment

josephaxisa Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2021

APIX Tests

jkaster commented Oct 13, 2021

josephaxisa commented Oct 13, 2021

jkaster left a comment

Choose a reason for hiding this comment

bryans99 left a comment

Choose a reason for hiding this comment

bryans99 Oct 13, 2021

Choose a reason for hiding this comment

josephaxisa Oct 14, 2021

Choose a reason for hiding this comment

This comment has been minimized.

josephaxisa Oct 13, 2021 •

edited

Loading