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(gatsby): add useStaticQuery hook #11588

Merged
merged 47 commits into from
Feb 13, 2019
Merged

feat(gatsby): add useStaticQuery hook #11588

merged 47 commits into from
Feb 13, 2019

Conversation

sidharthachatterjee
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee commented Feb 6, 2019

Adds a new React Hook called useStaticQuery

Known Limitations

  • Only a single instance of useStaticQuery per file is supported at the moment

Todo

  • Add typings
  • Add hook implementation in browser entry
  • Add support for variables in queries
  • Add support for inlined queries
  • Add production babel transform
  • Add tests for production babel transform
  • Add conditional to check that hook is imported from gatsby and improve call expression checks
  • Add proper warning if useContext is undefined
  • Add warning if query var is undefined?
  • Remove import for useStaticQuery in babel transform
  • Remove query variable in babel transform
  • Add end to end tests for new hook
  • Fix TypeScript typings

This is good to go now! Documentation and support for multiple instances will be added in later.

@sidharthachatterjee sidharthachatterjee requested a review from a team as a code owner February 6, 2019 12:32
@sidharthachatterjee sidharthachatterjee changed the title feat(gatsby): add hook for useStaticQuery [WIP] feat(gatsby): add hook for useStaticQuery Feb 6, 2019
@@ -20,4 +20,6 @@ export interface StaticQueryProps {

export class StaticQuery extends React.Component<StaticQueryProps> {}

export const useStaticQuery: (query: any) => void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could probably improve typings here since query needs to be graphql tagged template

Copy link
Contributor

Choose a reason for hiding this comment

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

It also should return Object (definitely not void, as this would probably make TS complain when using result)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, missed that! It can be null or obj though

Copy link
Contributor

Choose a reason for hiding this comment

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

@sidharthachatterjee good call! I think I'd just do

export const useStaticQuery: (query: any) => null | object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the return here

I'll fix the input for both StaticQuery and useStaticQuery in a separate PR

It looks like they should be TemplateStringsArray but don't want to touch StaticQuery in this PR

@sidharthachatterjee
Copy link
Contributor Author

sidharthachatterjee commented Feb 6, 2019

I’ve currently set the default return for useStaticQuery to be null

Not sure what the pattern is here with hooks yet but null would mean the user would need to do null checks themselves which the render prop component does for them.

What do we think of this? @KyleAMathews @pieh @m-allanson @DSchau

Copy link
Contributor

@DSchau DSchau 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 looking awesome! Left some preliminary comments, and then I'll pull this down and validate later, as well!

@@ -1,4 +1,4 @@
import React from "react"
import React, { useContext } from "react"
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a functional change necessarily, but maybe just use React.useContext instead? We can check for the existence of that rather easily, e.g.

if (typeof React.useContext !== `function` && process.env.NODE_ENV === `development`) {
  // warn
}

Copy link
Contributor

@DSchau DSchau Feb 6, 2019

Choose a reason for hiding this comment

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

Also one note here... I think it's worth validating that these checks are stripped from our code in production (e.g. with our uglifier).

(not as a concern of this PR, speaking more broadly!)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is tricky, because I would want this error to print out during built-html stage (when we write to static html files), but not actually end up in client js bundle (just micro optim to save few bytes)

Copy link
Contributor Author

@sidharthachatterjee sidharthachatterjee Feb 6, 2019

Choose a reason for hiding this comment

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

Added the check in gatsby-browser-entry.js

@pieh This will never end up in the bundle because I'll be stripping away the import and replacing it with static data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All imports to useStaticQuery are now removed in build so this should be good

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you are completely right - so this check would need to be done in babel plugin? (yuck - mixed concerns)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I guess it would need to be duplicated - in case there are 3rd party tools (Storybook etc) that want to use gatsby-browser-entry outside of gatsby "realm"?

@@ -20,4 +20,6 @@ export interface StaticQueryProps {

export class StaticQuery extends React.Component<StaticQueryProps> {}

export const useStaticQuery: (query: any) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

@sidharthachatterjee good call! I think I'd just do

export const useStaticQuery: (query: any) => null | object

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Let's get the tests passing--otherwise I think this is good to go!

@sidharthachatterjee sidharthachatterjee changed the title feat(gatsby): add useStaticQuery hook [WIP] feat(gatsby): add useStaticQuery hook Feb 8, 2019
@pieh
Copy link
Contributor

pieh commented Feb 8, 2019

@vadistic We were doing some checks and yeah, generics are definitely way to go. Remaining question is default type for query results - it seems like using object isn't ideal either:

error TS2339: Property 'foo' does not exist on type 'object'.

64     const test = data.foo
                         ~~~

this in practice forces users to either provide their type definition or use any - maybe we should just default to any?

@pieh
Copy link
Contributor

pieh commented Feb 8, 2019

Current blocker:
There is potential timing issue in gatsby develop and we could render page before static query results are available in StaticQueryContext. This might be unlikely but definitely possible. For test I added timeout before emitting static query results in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/websocket-manager.js#L132-L138 which resulted in this:

kapture 2019-02-08 at 18 58 44
For <StaticQuery> component this is acceptable because we render Loading state, but we can't do the same for useStaticQuery hook. This can only happen on initial render (or if there are query errors, but then solution is to fix queries) and can cause clientside errors if null checks for query results aren't added.

  • @pieh to open separate issue to discuss potential solutions.

@vadistic
Copy link

vadistic commented Feb 8, 2019

@pieh good point, skipped my mind :)

Query response will always be string indexed object extending scalar GraphQL types or arrays of those. We could provide index signatures for the first level but yeah, just go with any.

As in my link above - react-apollo does that (350k+ downloads weekly + it's typescript project), so I'm pretty confident it can be considered as standard practice.

@sidharthachatterjee sidharthachatterjee changed the title [WIP] feat(gatsby): add useStaticQuery hook feat(gatsby): add useStaticQuery hook Feb 13, 2019
) {
report.panicOnBuild(
`You're likely using a version of React that doesn't support Hooks.
Please update to React 16.8.0 or later to use the useStaticQuery hook.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! Fixed.

@sidharthachatterjee
Copy link
Contributor Author

Documentation for this is added in #11741

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM

@pieh pieh merged commit f149c4c into master Feb 13, 2019
@jlengstorf jlengstorf deleted the feat/static-query-hook branch February 13, 2019 18:51
gurpreet-hanjra pushed a commit to gurpreet-hanjra/gatsby that referenced this pull request Feb 14, 2019
m-allanson added a commit to gurpreet-hanjra/gatsby that referenced this pull request Feb 15, 2019
* master: (76 commits)
  chore(release): Publish
  Update relay version once again (gatsbyjs#11767)
  Update CHANGELOG.md (gatsbyjs#11764)
  chore(release): Publish
  Move to @gatsbyjs scoped version of yarn (gatsbyjs#11759)
  fix(blog): 2019-01-01 json code blocks (gatsbyjs#11750)
  fix(starters): update dependency gatsby to ^2.1.0 (gatsbyjs#11745)
  fix(starters): update dependency prop-types to ^15.7.2 (gatsbyjs#11748)
  feat(showcase): add Incremental.com.au (gatsbyjs#11729)
  feat(starters): add starter magicsoup.io (gatsbyjs#11670)
  docs(gatsby): Add documentation for useStaticQuery (gatsbyjs#11741)
  chore(release): Publish
  feat(gatsby): add useStaticQuery hook (gatsbyjs#11588)
  chore(release): Publish
  chore(docs): reword CSS in JS docs for clarity (gatsbyjs#11439)
  chore: Upgrade Prettier related packages to the latest (gatsbyjs#11735)
  fix(core): added event source polyfill in develop to fix IE/edge hmr (gatsbyjs#11582)
  chore: minify svg husky hook (gatsbyjs#10560)
  docs: add videos for Gatsby Link + rewrite for flow (gatsbyjs#11700)
  docs: add egghead lesson to quickstart (gatsbyjs#11699)
  ...
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.

8 participants