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(docs): API sandbox #517

Merged
merged 19 commits into from
Apr 30, 2020
Merged

Conversation

friej715
Copy link
Contributor

I think this PR could use some eyes (and maybe in the end we won't need it), but I wanted to put it out there and get some comments sooner rather than later!

I took a pass at writing up a spec for the v1 API, and made a little sandbox page (based on swagger-ui-react) that lays out the different schema/endpoints, and lets folks try executing queries.

I think ultimately we would want to tie the API and the docs closer together, so there's not two sources of truth (this is just pulling from a json file that I wrote in Swagger)--but this might be a handy in-between til then.

@kevee
Copy link
Member

kevee commented Apr 13, 2020

Hi, @friej715, thanks for this! It looks like gatsby build is failing because there's no window object when it does SSR. Try adding this under exports.onCreateWebpackConfig to gatsby-node.js. This makes the SSR process ignore swagger when building:

exports.onCreateWebpackConfig = ({ stage, actions, loaders, getConfig }) => {
  ........stuff that is already there....
  if (stage === 'build-html') {
    setWebpackConfig({
      module: {
        rules: [
          {
            test: /swagger-ui/,
            use: loaders.null(),
          },
        ],
      },
    })
  }
}

@friej715
Copy link
Contributor Author

Thank you @kevee, on it!

One more thing I wanted to offer: there's currently no contextualizing content on this page. If the content folks (@kissane et al) think it'd be a good idea to have some copy there, I could pull something from Contentful like the other pages do.

@muamichali
Copy link
Contributor

@friej715 this looks great! I imagine that it could replace part (or all) of this page https://covidtracking.com/api is that different than what others were thinking?

@friej715
Copy link
Contributor Author

friej715 commented Apr 16, 2020

(I just learned about draft PRs today, so I'm changing this to that!)

I think I've fixed the issues in gatsby-node that was blocking this from building at first, but I've been hung up on an error with gatsby build that I could use some help with. It feels like it has an obvious answer.

Every time I try to build, I get: Building static HTML failed for path "/data/api-sandbox", with the react error decoder saying Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object. This goes away if I replace the SwaggerUI component (in swagger-sandbox.js) with a plain div. Clearly there's something wonky happening in that file/with that component, and I'm not returning the right thing, but I can't see any differences between what I'm doing there and anything else, and none of my attempted solutions have worked so far.

The only thing that comes to mind is that swagger-ui-react may not play nicely with Gatsby, and maybe I'm misunderstanding something about the Gatsby build process. Any thoughts @kevee or @schwartzadev?

@superbiche
Copy link
Contributor

superbiche commented Apr 22, 2020

@friej715 OK so I did some search and testing, it seems Swagger UI doesn't play well with SSR and it's more an issue on their side than Gatsby. See gatsbyjs/gatsby#11081 (comment)

Basically, the workaround is to avoid rendering it in SSR, then render it - client-only - using componentDidMount.

Which actually seems like a good idea on the performance side of things: rendering anything related to Swagger UI in SSR would make the bundle (probably much) heavier and I'm not sure that's relevant for the sandbox API docs (while it might still make sense to do so for the production API, thus making it quite difficult to replace the /api page with a Swagger UI like @muamichali suggested)

I quickly tried the mentioned solution and it works. If this seems like a reasonable workaround, check out friej715#1

And also I removed the null loader workaround as it's not needed anymore with this approach.

EDIT: if we go down this road, it would be a good idea to load Swagger UI dist CSS only on the client too

@friej715
Copy link
Contributor Author

friej715 commented Apr 22, 2020

Thank you so much @superbiche, that PR looks promising! Almost clapped with glee when gatsby build was successful. @kevee knows a lot more about these things than I do, so I'm going to let him have final say.

I think the only change I'd make (if my git understanding is right) is just to switch the branch your PR is against, so that it's merging into my fork's jane/api-sandbox rather than master. That's the branch this PR is against, so I think I could then just merge it into jane/api-sandbox and the updates should pop right into this PR.

@superbiche
Copy link
Contributor

@friej715 will switch the target branch, just chatted with Kai a few minutes ago about me lost regarding where to PR what 😅

@friej715
Copy link
Contributor Author

friej715 commented Apr 22, 2020

Great, thank you @superbiche! Looks like some tests are still failing--I think maybe because we just updated a bunch of testing stuff within the last 24-48h. I can take a look later tonight.

@superbiche
Copy link
Contributor

@friej715 OK let me know if you need further help, I'll be working on another PR for the next couple hrs

@netlify
Copy link

netlify bot commented Apr 23, 2020

This is a preview version of the site.

Built with commit 608408f

https://deploy-preview-517--upbeat-lovelace-3e9fff.netlify.app

@friej715 friej715 marked this pull request as ready for review April 27, 2020 15:25
@friej715 friej715 requested a review from schwartzadev April 27, 2020 15:25
@kevee kevee self-requested a review April 27, 2020 15:30
@kevee
Copy link
Member

kevee commented Apr 27, 2020

⚠️Hold off on merge until @friej715 reviews with other stakeholders

Post go-live:

  • Add link to sandbox in API page

@friej715
Copy link
Contributor Author

friej715 commented Apr 27, 2020

Quickly popping on to say--I think the code here is largely good to go, but I'd love to have @webmasterkai and/or @kissane (or anyone on their respective API/content teams) check over the descriptions etc. and make sure everything is described accurately. You can see the preview here.

Copy link
Collaborator

@schwartzadev schwartzadev left a comment

Choose a reason for hiding this comment

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

This looks excellent! Just one question 😄.

@@ -1,5 +1,9 @@
const SwaggerUI = require('swagger-ui')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that Swagger runs on every page? If do, is there a way to only load it on the pages we'll need it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I think @superbiche might know?

Copy link
Member

Choose a reason for hiding this comment

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

Wow, I missed that. I'm going to do a test and propose a change to just load swagger on that one page.

Copy link
Member

Choose a reason for hiding this comment

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

OK I just pushed a commit that:

  • Uses onCreateWebpackConfig to say "Gatsby, when you're building the site with SSR, and therefore there is no window object, just chill and don't bother about Swagger."
  • Loads swagger UI in the SwaggerSandbox component.

@schwartzadev
Copy link
Collaborator

Maybe this is a silly question, but will this replace the current API page?

If so, we should definitely include the preamble that's on there now and make sure this PR includes all of the endpoints listed there (counties, tracker URLs, etc.)

@friej715
Copy link
Contributor Author

@schwartzadev Not sure about whether this is in addition to or replacing the other API docs--we've talked about it some but I don't think there's been one definitive answer.

As for the second--makes sense! I wrote these docs like two weeks ago, which is basically forever in covid19 tracking time 🤣 I'll get on that later tonight when I'm done with work.

@kevee
Copy link
Member

kevee commented Apr 28, 2020

My thought is that this should replace the API page, but that can happen slowly - we do this merge and I can update the API page in Contentful and point it at this one for now.

Long term, I think the API page should be a snippet from Contentful, plus a swagger component.

Copy link
Member

@kevee kevee left a comment

Choose a reason for hiding this comment

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

This is such amazing work @friej715! I'm so happy to get this change in today.

@kevee kevee merged commit 589b278 into COVID19Tracking:master Apr 30, 2020
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.

5 participants