Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: export reused api explorer functionality from root index.ts #890

Merged
merged 9 commits into from
Nov 4, 2021

Conversation

josephaxisa
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

APIX Tests

0 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌

Results for commit 7e41ec7.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

APIX Tests

    1 files    77 suites   2m 59s ⏱️
322 tests 309 ✔️ 13 💤 0 ❌
332 runs  319 ✔️ 13 💤 0 ❌

Results for commit 05d633d.

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

Looking good so far. I'll hold final approval for when it's out of draft mode.

  • Running yarn test:apix I get a warning about an improper teardown, but yarn test:apix --detectOpenHandles doesn't show any warnings or details at all
  • Running yarn dev:apix I get:
Uncaught TypeError: Failed to construct 'URL': Invalid base URL
at App.tsx:43

Comment on lines 26 to 28
export * from './ApiExplorer'
export { Loader } from './components'
export { store } from './state'
Copy link
Contributor

Choose a reason for hiding this comment

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

mix of * and named exports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll switch to named exports from 'ApiExplorer' for consistency

@@ -32,10 +32,8 @@ import type { ExtensionContextData } from '@looker/extension-sdk-react'
import { ExtensionContext } from '@looker/extension-sdk-react'
import type { SpecItem, SpecList } from '@looker/sdk-codegen'
import { getSpecsFromVersions } from '@looker/sdk-codegen'
import ApiExplorer from '@looker/api-explorer/src/ApiExplorer'
import { Loader } from '@looker/api-explorer/src/components'
import { ApiExplorer, store, Loader } from '@looker/api-explorer'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

@josephaxisa
Copy link
Contributor Author

@jkaster yarn dev:apix works fine on my end, but yarn dev:xapix does not. Seems like the test issue has been there a while and is unrelated to these changes.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2021

APIX Tests

    1 files    77 suites   2m 49s ⏱️
322 tests 309 ✔️ 13 💤 0 ❌
332 runs  319 ✔️ 13 💤 0 ❌

Results for commit 6b6ded5.

bryans99
bryans99 previously approved these changes Nov 3, 2021
Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

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

LGTM aside from export {} which we've had issues with.

I have not tested though and wont have a chance to

SOFTWARE.

*/
export { ApiExplorer } from './ApiExplorer'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any compile issues with this? We've had to use export * elsewhere. Are there any other exports? if no consider switching to export *

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2021

APIX Tests

    1 files    77 suites   2m 52s ⏱️
322 tests 309 ✔️ 13 💤 0 ❌
332 runs  319 ✔️ 13 💤 0 ❌

Results for commit b1f2933.

@jkaster jkaster self-requested a review November 3, 2021 20:47
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2021

APIX Tests

    1 files    77 suites   3m 18s ⏱️
322 tests 309 ✔️ 13 💤 0 ❌
332 runs  319 ✔️ 13 💤 0 ❌

Results for commit 151cb79.

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM now. Tested both extension and browser environment

@josephaxisa josephaxisa marked this pull request as ready for review November 4, 2021 13:57
@josephaxisa josephaxisa merged commit fb8634f into main Nov 4, 2021
@josephaxisa josephaxisa deleted the jax/apix-exports branch November 4, 2021 14:16
@github-actions

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants