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.

Sign up for GitHub

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

Jump to bottom

feat: two-way type references #684

Merged
merged 5 commits into from
May 14, 2021
Merged

feat: two-way type references #684

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d253a1d
feat: add two-way type references
josephaxisa May 14, 2021
c5daa46
Merge branch 'main' into jax/two-way-type-refs
josephaxisa May 14, 2021
5d77aed
feat: show two-way type refs in api explorer
josephaxisa May 14, 2021
8f35b37
test for complex hash types
josephaxisa May 14, 2021
6119e97
Merge branch 'main' into jax/two-way-type-refs
josephaxisa May 14, 2021
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
28 changes: 18 additions & 10 deletions packages/api-explorer/src/components/DocReferences/DocReferences.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,34 +33,42 @@ import { DocReferences } from './DocReferences'

describe('DocReferences', () => {
test('it renders method and type references', () => {
const seeTypes = typeRefs(api, api.types.Dashboard.customTypes)
const seeMethods = methodRefs(api, api.types.Dashboard.methodRefs)
const typesUsed = typeRefs(api, api.types.DashboardElement.customTypes)
const typesUsedBy = typeRefs(api, api.types.DashboardElement.parentTypes)
const methodsUsedBy = methodRefs(api, api.types.DashboardElement.methodRefs)
renderWithSearchAndRouter(
<DocReferences
seeTypes={seeTypes}
seeMethods={seeMethods}
typesUsed={typesUsed}
typesUsedBy={typesUsedBy}
methodsUsedBy={methodsUsedBy}
specKey={'3.1'}
api={api}
/>
)
expect(screen.getAllByRole('link')).toHaveLength(
seeTypes.length + seeMethods.length
typesUsed.length + typesUsedBy.length + methodsUsedBy.length
)
expect(screen.getByText(seeTypes[0].name).closest('a')).toHaveAttribute(
expect(screen.getByText(typesUsed[0].name).closest('a')).toHaveAttribute(
'href',
`/3.1/types/${seeTypes[0].name}`
`/3.1/types/${typesUsed[0].name}`
)
expect(screen.getByText(seeMethods[0].name).closest('a')).toHaveAttribute(

expect(typesUsedBy).toHaveLength(1)
expect(typesUsedBy[0].name).toEqual('Dashboard')
expect(screen.getByText(typesUsedBy[0].name).closest('a')).toHaveAttribute(
'href',
`/3.1/methods/Dashboard/${seeMethods[0].name}`
`/3.1/types/${typesUsedBy[0].name}`
)
expect(
screen.getByText(methodsUsedBy[0].name).closest('a')
).toHaveAttribute('href', `/3.1/methods/Dashboard/${methodsUsedBy[0].name}`)
})

test('it highlights text matching search pattern', () => {
const highlightPattern = 'dash'
renderWithSearchAndRouter(
<DocReferences
seeTypes={[api.types.Dashboard]}
typesUsed={[api.types.Dashboard]}
specKey={'3.1'}
api={api}
/>,
Expand Down
28 changes: 21 additions & 7 deletions packages/api-explorer/src/components/DocReferences/DocReferences.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ import { CollapserCard } from '../Collapser'
import { DocReferenceItems } from './utils'

interface DocReferencesProps {
seeTypes: IType[]
seeMethods?: IMethod[]
typesUsed: IType[]
methodsUsedBy?: IMethod[]
typesUsedBy?: IType[]
specKey: string
api: ApiModel
}
Expand All @@ -43,31 +44,44 @@ interface DocReferencesProps {
* It renders links to the given types and/or methods references
*/
export const DocReferences: FC<DocReferencesProps> = ({
seeTypes,
typesUsed,
specKey,
api,
seeMethods = [],
methodsUsedBy = [],
typesUsedBy = [],
}) => {
const {
searchSettings: { pattern },
} = useContext(SearchContext)

if (seeTypes.length === 0 && seeMethods.length === 0) return <></>
if (
typesUsed.length === 0 &&
methodsUsedBy.length === 0 &&
typesUsedBy.length === 0
)
return <></>

return (
<Box id="references" mb="xlarge">
<CollapserCard heading="References">
<>
{DocReferenceItems(
'Referenced Types:',
seeTypes,
typesUsed,
api,
specKey,
pattern
)}
{DocReferenceItems(
'Used by types:',
typesUsedBy,
api,
specKey,
pattern
)}
{DocReferenceItems(
'Used by methods:',
seeMethods,
methodsUsedBy,
api,
specKey,
pattern
Expand Down
2 changes: 1 addition & 1 deletion packages/api-explorer/src/scenes/MethodScene/MethodScene.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export const MethodScene: FC<DocMethodProps> = ({ api }) => {
<DocSDKs api={api} method={method} />
<DocRequestBody method={method} />
<DocSdkUsage method={method} />
<DocReferences seeTypes={seeTypes} api={api} specKey={specKey} />
<DocReferences typesUsed={seeTypes} api={api} specKey={specKey} />
<DocResponses responses={method.responses} />
</ApixSection>
{sdk && value && (
Expand Down
10 changes: 6 additions & 4 deletions packages/api-explorer/src/scenes/TypeScene/TypeScene.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ interface DocTypeParams {
export const TypeScene: FC<DocTypeProps> = ({ api }) => {
const { specKey, typeName } = useParams<DocTypeParams>()
const type = api.types[typeName]
const seeTypes = typeRefs(api, type.customTypes)
const seeMethods = methodRefs(api, type.methodRefs)
const typesUsed = typeRefs(api, type.customTypes)
const methodsUsedBy = methodRefs(api, type.methodRefs)
const typesUsedBy = typeRefs(api, type.parentTypes)

return (
<ApixSection>
Expand All @@ -61,8 +62,9 @@ export const TypeScene: FC<DocTypeProps> = ({ api }) => {
</Space>
<ExploreType type={type} />
<DocReferences
seeTypes={seeTypes}
seeMethods={seeMethods}
typesUsed={typesUsed}
typesUsedBy={typesUsedBy}
methodsUsedBy={methodsUsedBy}
api={api}
specKey={specKey}
/>
Expand Down
12 changes: 12 additions & 0 deletions packages/sdk-codegen/src/sdkModels.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,18 @@ describe('sdkModels', () => {

describe('method and type xrefs', () => {
describe('custom types', () => {
it('determines parent type of array types', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a test for "parent of hash types" since your fix in sdkModels is

propType.instanceOf('ArrayType') || propType.instanceOf('HashType')

(or conversely remove the || HashType condition)

Copy link
Contributor

Choose a reason for hiding this comment

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

We have no complex hash types in the model right now, so we need to create a sample spec for that.

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 didn't add a test for this because unfortunately we do not have any typed HashType in the spec. All parameters of type IDictionary expect a scalar property value. I added the check because I expect it to work when we do, but perhaps I should remove it so that when we do need it, we'll remember to add a test

Copy link
Contributor

Choose a reason for hiding this comment

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

we've modified test/openApiRef.json in the past to add spec schema that we don't already have in Looker: 1b64e1b#diff-90f6053ccc4c15adb9cbbba6833dbfb5a6206c6fb3793fe206f10f71230b73faR29331

So yeah, I'd say either remove the check (perhaps adding a TODO) or add the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the test

const arrayType = apiTestModel.types.DashboardElement
const parents = arrayType.parentTypes
expect(parents).toEqual(new Set(['DashboardElement[]', 'Dashboard']))
})

it('determines parent type of enum types', () => {
const enumType = apiTestModel.types.LinkedContentType
const parents = enumType.parentTypes
expect(parents).toEqual(new Set(['Command']))
})

it('intrinsic types have undefined custom types', () => {
const actual = new IntrinsicType('integer')
expect(actual.customType).toEqual('')
Expand Down
8 changes: 5 additions & 3 deletions packages/sdk-codegen/src/sdkModels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1541,10 +1541,12 @@ export class Type implements IType {
// Track the "parent" reference for this type from the property reference
propType.parentTypes.add(this.name)
if (
propType.instanceOf('ArrayType') &&
propType.elementType?.instanceOf('EnumType')
propType.instanceOf('ArrayType') ||
propType.instanceOf('HashType')
) {
propType.elementType.parentTypes.add(propType.name)
propType.elementType?.parentTypes.add(propType.name)
propType.elementType?.parentTypes.add(this.name)
Comment on lines +1547 to +1548
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we want both DashboardElement[] and Dashboard? It seems to me that IType.{types|parentTypes} is meant to reference other IType objects which I believe (though I could be wrong) are anything that would be in a "$ref" value from the spec. I don't think DashboardElement[] satisfies that requirement. In fact, in APIX you're using typeRefs(...) which appears to filter out anything that isn't a "$ref" (and thus DashboardElement[] is removed)?

I don't know which of these two lines puts in DashboardElement[] but I think it should be removed.

Caveat: not super up to speed on the actual details/usage of the SdkModel so I could be totally wrong here.

Copy link
Contributor

Choose a reason for hiding this comment

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

parentTypes is primarily for APIX explorer documentation purposes to link a type to the types that use or "contain" it. For exploration, we don't bother to show an array collection type, just the underlying complex type.

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 is a good point and would only require some additional validation in sdkModels.ts. I'm unsure whether we should do that or not though, I'll have to defer to @jkaster

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't answer completely. I think DashboardElement should have parent types that are Dashboard and DashboardElement[] because both "contain" DashboardElement.

Copy link
Contributor

@joeldodge79 joeldodge79 May 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I guess my more basic question is what what is a "type" in the context here.

  • this is an instance of Type that implements IType
  • IType.{types|parentTypes}is currently just Set<string> but it seems to me like they're more meant to be Set<IType> (which I know doesn't actually work under the hood due to object equality checks on a set).

If that's correct (i.e. type/parentTypes is mean to represent a collection of IType) then what is an IType? A Dashboard is an IType but DashboardElement[] is not. The former exists as a value to a $ref key in the schema where as the latter never does, rather DashboardElement is referenced in the items key of another IType's property that has a type value of "array"?

On the other hand, maybe types/parentTypes is not a loose reference to IType but rather meant to be the union of IType and IType[]? But, as you say it's primarily for APIX which promptly discards the IType[] type by calling typeRefs(...)?

Or maybe I'm just up too early and my brain isn't on :-) happy to hash out in person later

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I thought that might be the question. They must be Set<string> rather than IType to prevent circular references that break JSON serialization. You'll find we use the name of the type rather than the actual Type in several collections for this reason.

Copy link
Contributor

@joeldodge79 joeldodge79 May 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not doing a good job expressing my question/concern: it's not about the implementation of the typescript type or javascript runtime implementation of IType.{types|parentTypes}: my question/concern is what kinds of "things" do IType.{types|parentTypes} refer to. I believe they refer to IType things (even though there's no typescript type constraint because we're just using Set<string>), not container-datastructures of IType things and hence DashboardElement[] does not belong.

I'm thinking of an ERD - a parentType would be another box but would not also include the many-to-one line-arrow pointing to that box

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I see DashboardElement[] and Dashboard as two fundamentally different kinds of things:

Dashboard - a real thing (a User is a real thing etc)
DashboardElement[] - a programming construct to describe a collection of real things

Housing those under the same property feels like saying "one of my parents is named Tom, and I have another parent whose name is 'Tom's kids'"

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it gets muddy. What do you suggest instead? We definitely need to link child types of a collection to the user of said collection. For visual representation in APIX, this works. Can we create a ticket to work out this design and address it in another PR, because I've already thought about this and haven't come up with anything clearly better yet, and for the purpose of APIX type ref navigation this works.

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely need to link child types of a collection to the user of said collection

I don't totally understand what this means w/out seeing concrete code but I'll take your word for it: carry on :-)

propType.parentTypes.add(this.name)
}
this.types.add(propType.name)
const customType = propType.customType
Expand Down