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(api): make API component resolve inline and remote refs #575

Merged
merged 13 commits into from
Sep 29, 2020

Conversation

marcelltoth
Copy link
Contributor

Resolves #570

Inline ref resolution is in place, remote resolution is TBD.

@marcelltoth
Copy link
Contributor Author

Taken over by @mmiask

@mmiask mmiask marked this pull request as ready for review September 17, 2020 12:51
@mmiask mmiask requested a review from lottamus as a code owner September 17, 2020 12:51
@mmiask mmiask requested a review from a team September 17, 2020 12:51
using bundled data instead of dereferenced in API container
@mmiask mmiask requested a review from P0lip September 21, 2020 09:14
@@ -0,0 +1,47 @@
import $RefParser from '@stoplight/json-schema-ref-parser';
import { NodeType } from '@stoplight/types';
import { isObject } from 'lodash';
Copy link
Contributor

@P0lip P0lip Sep 21, 2020

Choose a reason for hiding this comment

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

Suggested change
import { isObject } from 'lodash';
import { isObjectLike } from 'lodash';

isObject returns true for functions.

(or isPlainObject)

We can leave it as well too, as I doubt function will ever be provided.

Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

👍

@marcelltoth
Copy link
Contributor Author

Putting this on hold until the discussion is settled in #570.

@marcelltoth marcelltoth marked this pull request as draft September 22, 2020 10:16
@marcelltoth marcelltoth marked this pull request as ready for review September 25, 2020 09:47
@mmiask mmiask requested a review from mallachari September 28, 2020 12:21
@mmiask
Copy link
Contributor

mmiask commented Sep 28, 2020

@mallachari I modified some files related to StackedLayout and SidebarLayout that you introduced recently. Could you please take a look at these?

@netlify
Copy link

netlify bot commented Sep 28, 2020

Deploy preview for stoplight-elements ready!

Built with commit 5792351

https://deploy-preview-575--stoplight-elements.netlify.app

Copy link
Contributor Author

@marcelltoth marcelltoth left a comment

Choose a reason for hiding this comment

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

There is a lot of passing around of the base URL and the document, and we're bundling all over the place.

Is there anything preventing us to do the bundling once at the container level then be done with it?

Done correctly, that would even reduce your diff size, currently there's quite some boilerplate and duplicated logic.

</Tabs>
) : (
<Docs className="mx-auto p-4" nodeType={nodeType} nodeData={data} headless />
<InlineRefResolverProvider document={document}>

Choose a reason for hiding this comment

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

As this is a context we shouldn't need to pass document all the way down with props. Did you try to put InlineRefResolverProvider at the StackedLayout root level?

Copy link
Contributor Author

@marcelltoth marcelltoth left a comment

Choose a reason for hiding this comment

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

Almost there. Much less redundant data passing down, much more centralized resolving, good job.

There is still some redundant data being passed down. nodeType, bundledNodeData, uriMap, they all refer to mostly the same information.

The simplest way to think about the problem would be to bundle all the $refs into the document on the container level before we start processing it, then the processing logic doesn't need to change at all.

linkComponent?: React.ComponentType<ILinkComponentProps>;
apiDescriptionUrl?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the cleaned up implementation this was made redundant here, we're not using it anywhere. Let's remove it.

@@ -26,7 +27,7 @@ const itemMatchesHash = (hash: string, item: Pick<ItemRowProps, 'title' | 'type'
return hash.substr(1) === `${item.title}-${item.type}`;
};

export const StackedLayout: React.FC<StackedLayoutProps> = ({ uriMap, tree }) => {
export const StackedLayout: React.FC<StackedLayoutProps> = ({ uriMap, tree, bundledNodeData }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheating 😛
WebStorm immediately gave me an unused variable warning. And it is right, trying your external ref file: https://raw.githubusercontent.com/mmiask/anothertesting/master/reference/test.v1.yaml with the stacked layout crashes immediately upon opening the Endpoints group.

@marcelltoth
Copy link
Contributor Author

I pushed some changes that implement the centralized bundling I was imagining. Let me know what you think @mmiask @mallachari

Even with some bugfixes added in the bundling hook it still reduced the diff from +109-31 & 7 files to +93-26 & 5 files as the presentational components are now ignorant of the bundling logic.

@mmiask
Copy link
Contributor

mmiask commented Sep 29, 2020

@marcelltoth I was just about to push my changes that used React.Context to pass the bundled document, but your solutions looks just as good. Looking at it right now

@mallachari
Copy link

I pushed some changes that implement the centralized bundling I was imagining. Let me know what you think @mmiask @mallachari

Even with some bugfixes added in the bundling hook it still reduced the diff from +109-31 & 7 files to +93-26 & 5 files as the presentational components are now ignorant of the bundling logic.

It's more compact now, I like it this way.

@marcelltoth marcelltoth merged commit f040968 into beta Sep 29, 2020
@marcelltoth marcelltoth deleted the feat/resolve-refs branch September 29, 2020 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API component should handle remote and inline $refs
5 participants