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

Wrap window references for SSR #531

Merged
merged 4 commits into from
Jun 8, 2019
Merged

Wrap window references for SSR #531

merged 4 commits into from
Jun 8, 2019

Conversation

jaronheard
Copy link
Contributor

Addresses #503

Still requires the Webpack configuration tweak to work with Gatsby, as described in their documentation on this issue. I feel like there is a workaround for the two offending modules, but I'm not sure what it is.

I added the global module as a dependency for this purpose.

exports.onCreateWebpackConfig = ({ stage, loaders, actions }) => {
  if (stage === "build-html") {
    actions.setWebpackConfig({
      module: {
        rules: [
          {
            test: /react-map-gl-geocoder/,
            use: loaders.null()
          },
          {
            test: /mapbox-gl/,
            use: loaders.null()
          }
        ]
      }
    });
  }
};

@ghost ghost self-requested a review May 22, 2019 22:10
@ghost ghost self-assigned this May 22, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I found some potential errors. Also, I'm not able to test on CodeSandbox for some reason. The terminal runs to here:
Screen Shot 2019-05-22 at 3 53 17 PM
So I haven't been able to test how this works. I don't see any gatsby config files in the repo, just on the sandbox. I'm guessing in lieu of using sandbox I could install the gatsby-cli?

@@ -12,21 +13,21 @@ class CanvasParticles extends React.Component {
componentDidMount() {
window.requestAnimFrame = (function() {
Copy link

Choose a reason for hiding this comment

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

Do you not need to do a check that window has requestAnimFrame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is setting a property on window, which we've imported from global, rather than getting a property, it should be fine

@ghost ghost assigned jaronheard and unassigned ghost May 22, 2019
@jaronheard jaronheard assigned ghost and unassigned jaronheard May 23, 2019
@jaronheard
Copy link
Contributor Author

@Dianna ready when you have a chance to take another look

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

The small issues I found have been fixed 👍

@jaronheard jaronheard merged commit 2ec50a3 into master Jun 8, 2019
@jaronheard jaronheard deleted the wrap-component-library branch July 9, 2019 03:51
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.

1 participant