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

feat: SDK language selection as URL search parameter #1123

Merged
merged 31 commits into from
Aug 9, 2022
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
730271f
SDK parameter in URL and new queryParams type in navigate
patnir41 Jul 22, 2022
b1c0af9
Added sdk parameter to object passed into navigate
patnir41 Jul 26, 2022
78ca5f5
Fixed/removed case for empty object, to be disucssed
patnir41 Jul 26, 2022
af4a710
Refactor commit wasn
patnir41 Jul 26, 2022
7897da3
Quick comment refactor
patnir41 Jul 26, 2022
93d5c93
Refactor to have URL drive state in APIExplorer, test additions
patnir41 Jul 28, 2022
8a9f8b4
Minor refactor
patnir41 Jul 28, 2022
92ab597
Comment per Bryn request
patnir41 Jul 29, 2022
00633a8
Refactor, enforce flow to URL and URL driving state, removed store re…
patnir41 Jul 29, 2022
7273536
Refactor to pass tests
patnir41 Jul 29, 2022
9c29e96
Fix to All option parameter showing
patnir41 Jul 30, 2022
16e3883
Fix and unit test for choosing SDK = All
patnir41 Aug 1, 2022
842926c
Pushing extension abbreviation to URL, fix state issues, added tests/…
patnir41 Aug 2, 2022
d7436d8
URL drives state over initActions, show sdk=all in URL due to state i…
patnir41 Aug 3, 2022
abc3555
Set initial URL and Explorer state based on initialized state in loca…
patnir41 Aug 4, 2022
8e96e15
Add back removed line
patnir41 Aug 4, 2022
6a25ce8
TypeScript error fix
patnir41 Aug 4, 2022
3170b7a
Removes sdk parameter on selecting All
patnir41 Aug 4, 2022
a7f0748
Significant optimizations and change to sdkLanguage storage
patnir41 Aug 5, 2022
d399430
Working e2e tests
patnir41 Aug 5, 2022
1898cc2
WIP comments from Joseph on e2e and sdkAlias selector
patnir41 Aug 5, 2022
d643f68
Passing e2e tests
patnir41 Aug 7, 2022
391a90b
Remove need for catching legacy store value
patnir41 Aug 7, 2022
141ee1b
Refactoring & cleanup
patnir41 Aug 8, 2022
047412b
Selector refactor, remove redundant logic
patnir41 Aug 8, 2022
7f9f872
Documentation for allSdkLanguages
patnir41 Aug 8, 2022
bf28b9b
Revert test checking default language selected
patnir41 Aug 8, 2022
3727b65
Remove unpredictability with e2e tests
patnir41 Aug 8, 2022
24ded0a
Refactoring and modifying e2e tests
patnir41 Aug 8, 2022
d8c3f51
allAlias instead of hardcode 'all'
patnir41 Aug 8, 2022
88c99a1
Merge branch 'main' into patnir41/sdk-sharing-url
patnir41 Aug 9, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions packages/api-explorer/e2e/diffScene.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ describe('Diff Scene', () => {
page.evaluate((el) => el.innerText.match(/^[a-z_]*/)[0], resultCard)
)
)
expect(page1Methods).toHaveLength(15)
expect(page1Methods).toHaveLength(16)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was failing in main, fixed so that it is up to date and accurately reflecting new additional method.

expect(page1Methods).toContain('delete_board_item')
}

Expand All @@ -176,13 +176,15 @@ describe('Diff Scene', () => {
const methodLink = await page.$(`${resultCardsSelector} a[role=link]`)
expect(methodLink).not.toBeNull()
const methodText = await page.evaluate((e) => e.innerText, methodLink)
expect(methodText).toMatch(`delete_board_item for 4.0`)
expect(methodText).toMatch(`delete_alert for 4.0`)

// Click and validate destination
await methodLink.click()
await page.waitForSelector(`div[class*=MethodBadge]`, { timeout: 5000 })
const compUrl = page.url()
expect(compUrl).toEqual(`${BASE_URL}/4.0/methods/Board/delete_board_item`)
expect(compUrl).toEqual(
`${BASE_URL}/4.0/methods/Alert/delete_alert?sdk=py`
)
}
})

Expand Down Expand Up @@ -217,7 +219,7 @@ describe('Diff Scene', () => {
// Check the URL
// Would like to do this earlier, but not sure what to wait on
const compUrl = page.url()
expect(compUrl).toEqual(`${BASE_URL}/diff/3.1/4.0`)
expect(compUrl).toEqual(`${BASE_URL}/diff/3.1/4.0?sdk=py`)

// Check the results
const diffResultCards = await page.$$(resultCardsSelector)
Expand All @@ -228,7 +230,7 @@ describe('Diff Scene', () => {
)
)

expect(diff31to40Page1Methods).toHaveLength(15)
expect(diff31to40Page1Methods).toHaveLength(16)
expect(diff31to40Page1Methods).toContain('delete_board_item')

// Click the switch button
Expand All @@ -245,7 +247,7 @@ describe('Diff Scene', () => {
await page.waitForTimeout(150)

const switchUrl = page.url()
expect(switchUrl).toEqual(`${BASE_URL}/diff/4.0/3.1`)
expect(switchUrl).toEqual(`${BASE_URL}/diff/4.0/3.1?sdk=py`)

// Check the results again, even though they should be the same
const diff40to31Page1Methods = await Promise.all(
Expand All @@ -254,7 +256,7 @@ describe('Diff Scene', () => {
)
)

expect(diff40to31Page1Methods).toHaveLength(15)
expect(diff40to31Page1Methods).toHaveLength(16)
expect(diff40to31Page1Methods).toContain('delete_board_item')
})
})
13 changes: 10 additions & 3 deletions packages/api-explorer/e2e/e2e.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('API Explorer', () => {
expect(page).toClick('h3', { text: 'Get All Dashboards' }),
])
await expect(page.url()).toEqual(
`${v40}/methods/Dashboard/all_dashboards`
`${v40}/methods/Dashboard/all_dashboards?sdk=py`
)

// title
Expand Down Expand Up @@ -164,6 +164,9 @@ describe('API Explorer', () => {
'Kotlin'
)
await expect(page).toMatchElement('h3', { text: 'Kotlin Declaration' })
await page.evaluate(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Observed this cause unpredictable behavior, new commit will move this to an afterEach within this test block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see the afterEach but this is still here. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The most recent commit maintains the intended structure, there are two afterEach's that handle the clearing of local storage.

localStorage.clear()
})
})

// This test was broken during the 4.0 GA spec changes, and needs to be fixed
Expand Down Expand Up @@ -255,7 +258,9 @@ describe('API Explorer', () => {
await expect(page).toMatchElement('button', { text: 'Types (0)' })
await expect(page).toClick('a', { text: 'Get Workspace' })
await expect(page).toMatchElement('h2', { text: 'Get Workspace' })
await expect(page.url()).toEqual(`${v40}/methods/Workspace/workspace`)
await expect(page.url()).toEqual(
`${v40}/methods/Workspace/workspace?sdk=py&s=get+workspace`
)
})

it('searches types', async () => {
Expand All @@ -267,7 +272,9 @@ describe('API Explorer', () => {
await expect(page).toClick('button', { text: 'Types (1)' })
await expect(page).toClick('a', { text: 'WriteTheme' })
await expect(page).toMatchElement('h2', { text: 'WriteTheme' })
await expect(page.url()).toEqual(`${v40}/types/Theme/WriteTheme`)
await expect(page.url()).toEqual(
`${v40}/types/Theme/WriteTheme?sdk=py&s=writetheme`
)
})
})
})
35 changes: 30 additions & 5 deletions packages/api-explorer/src/ApiExplorer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ import {
useSpecStoreState,
selectSpecs,
selectCurrentSpec,
selectSdkLanguage,
} from './state'
import { getSpecKey, diffPath } from './utils'
import { getSpecKey, diffPath, useNavigation, getSdkLanguage } from './utils'

export interface ApiExplorerProps {
adaptor: IApixAdaptor
Expand All @@ -85,19 +86,23 @@ export const ApiExplorer: FC<ApiExplorerProps> = ({
declarationsLodeUrl = `${apixFilesHost}/declarationsIndex.json`,
headless = false,
}) => {
useSettingStoreState()
const { initialized } = useSettingStoreState()
useLodesStoreState()
const { working, description } = useSpecStoreState()
const specs = useSelector(selectSpecs)
const spec = useSelector(selectCurrentSpec)
const selectedSdkLanguage = useSelector(selectSdkLanguage)
const { initLodesAction } = useLodeActions()
const { initSettingsAction, setSearchPatternAction } = useSettingActions()
const { initSettingsAction, setSearchPatternAction, setSdkLanguageAction } =
useSettingActions()
const { initSpecsAction, setCurrentSpecAction } = useSpecActions()

const location = useLocation()
const navigate = useNavigation()
const [hasNavigation, setHasNavigation] = useState(true)
const toggleNavigation = (target?: boolean) =>
setHasNavigation(target || !hasNavigation)
const searchParams = new URLSearchParams(location.search)

const hasNavigationToggle = useCallback((e: MessageEvent<any>) => {
if (e.origin === window.origin && e.data.action === 'toggle_sidebar') {
Expand All @@ -116,6 +121,24 @@ export const ApiExplorer: FC<ApiExplorerProps> = ({
return () => unregisterEnvAdaptor()
}, [])

useEffect(() => {
// reconcile local storage state with URL or vice versa
patnir41 marked this conversation as resolved.
Show resolved Hide resolved
if (initialized) {
const sdk = getSdkLanguage(searchParams.get('sdk'))
if (sdk) {
// sync store with URL
setSdkLanguageAction({
sdkLanguage: sdk.alias,
})
} else {
// sync URL with store
navigate(location.pathname, {
sdk: selectedSdkLanguage === 'all' ? null : selectedSdkLanguage,
})
}
}
}, [initialized])

useEffect(() => {
const maybeSpec = location.pathname?.split('/')[1]
if (spec && maybeSpec && maybeSpec !== diffPath && maybeSpec !== spec.key) {
Expand All @@ -124,9 +147,11 @@ export const ApiExplorer: FC<ApiExplorerProps> = ({
}, [location.pathname, spec])

useEffect(() => {
const searchParams = new URLSearchParams(location.search)
if (!initialized) return
const searchPattern = searchParams.get('s') || ''
setSearchPatternAction({ searchPattern: searchPattern! })
const sdkLanguage = searchParams.get('sdk') || 'all'
setSearchPatternAction({ searchPattern })
setSdkLanguageAction({ sdkLanguage })
}, [location.search])

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type { Store } from 'redux'
import { api } from '../../test-data'
import { renderWithReduxProvider, createTestStore } from '../../test-utils'
import type { RootState } from '../../state'
import { getSdkLanguage } from '../../utils'
import { DocSDKs } from './DocSDKs'

describe('DocSDKs', () => {
Expand Down Expand Up @@ -66,7 +67,10 @@ describe('DocSDKs', () => {
'it can render a %s method declaration',
(sdkLanguage) => {
store = createTestStore({
settings: { initialized: false, sdkLanguage },
settings: {
initialized: false,
sdkLanguage: getSdkLanguage(sdkLanguage)!.alias,
},
})
renderWithReduxProvider(
<DocSDKs api={api} method={api.methods.run_look} />,
Expand Down
13 changes: 7 additions & 6 deletions packages/api-explorer/src/components/DocSDKs/DocSDKs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import { CollapserCard, getGenerators } from '@looker/run-it'

import { DocCode } from '../DocCode'
import { selectSdkLanguage } from '../../state'
import { isMethod } from '../../utils/path'
import { isMethod, getSdkLanguage } from '../../utils'
import { noComment } from './utils'
import { DocDeclarations } from './DocDeclarations'

Expand Down Expand Up @@ -72,28 +72,29 @@ const getDeclarations = (
* Given a method or a type, it renders its SDK declaration in all supported languages.
*/
export const DocSDKs: FC<LanguageSDKProps> = ({ api, method, type }) => {
const sdkLanguage = useSelector(selectSdkLanguage)
const alias = useSelector(selectSdkLanguage)
const sdk = getSdkLanguage(alias)!
patnir41 marked this conversation as resolved.
Show resolved Hide resolved
const generators = getGenerators(api)
const [item, setItem] = useState(method ? noComment(method) : type!)
const [declarations, setDeclarations] = useState(
getDeclarations(generators, sdkLanguage, item)
getDeclarations(generators, sdk.language, item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Handy and simple. I like the composed value.

)
const [header, setHeader] = useState(`${sdkLanguage} Declaration`)
const [header, setHeader] = useState(`${sdk.language} Declaration`)

useEffect(() => {
setItem(method ? noComment(method) : type!)
}, [method, type])

useEffect(() => {
const declarations = getDeclarations(generators, sdkLanguage, item)
const declarations = getDeclarations(generators, sdk.language, item)
setDeclarations(declarations)
const languages = Object.keys(declarations)
if (languages.length > 1) {
setHeader('Declarations')
} else {
setHeader(`${languages[0]} Declaration`)
}
}, [sdkLanguage, item])
}, [sdk.language, item])

return (
<CollapserCard heading={header} id="sdk declarations">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import { InsertDriveFile } from '@styled-icons/material-outlined/InsertDriveFile
import { useSelector } from 'react-redux'

import { selectSdkLanguage, selectExamplesLode } from '../../state'
import { getSdkLanguage } from '../../utils'
import {
exampleColumns,
EMPTY_STRING,
Expand All @@ -61,7 +62,8 @@ interface DocSdkUsageProps {
*/
export const DocSdkUsage: FC<DocSdkUsageProps> = ({ method }) => {
const examples = useSelector(selectExamplesLode)
const sdkLanguage = useSelector(selectSdkLanguage)
const alias = useSelector(selectSdkLanguage)
patnir41 marked this conversation as resolved.
Show resolved Hide resolved
const sdk = getSdkLanguage(alias)!
patnir41 marked this conversation as resolved.
Show resolved Hide resolved
const [page, setPage] = useState(1)

useEffect(() => {
Expand All @@ -72,7 +74,7 @@ export const DocSdkUsage: FC<DocSdkUsageProps> = ({ method }) => {
let languages = findExampleLanguages(examples, method.name)
if (languages.length === 0) return <></>

languages = sortLanguagesByPreference(languages, sdkLanguage)
languages = sortLanguagesByPreference(languages, sdk.language)

const { pageExamples, pageLimit, paginate } = prepareExampleDataTable(
languages,
Expand Down
3 changes: 1 addition & 2 deletions packages/api-explorer/src/components/Header/Header.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import userEvent from '@testing-library/user-event'

import { getLoadedSpecs, specs } from '../../test-data'
import { renderWithRouterAndReduxProvider } from '../../test-utils'
import { defaultSettingsState } from '../../state'
import { Header } from './Header'

describe('Header', () => {
Expand Down Expand Up @@ -72,7 +71,7 @@ describe('Header', () => {
<Header spec={spec} toggleNavigation={toggleNavigation} />
)
const selector = screen.getByLabelText('sdk language selector')
expect(selector).toHaveValue(defaultSettingsState.sdkLanguage)
expect(selector).toHaveValue('Python')
await act(async () => {
await userEvent.click(selector)
await waitFor(() => {
Expand Down
Loading