Skip to content

Conversation

@rosahbruno
Copy link
Contributor

@rosahbruno rosahbruno commented Jul 25, 2023

Adds additional checks to verify window and localStorage object exist before making any calls. This handles errors that are ONLY generated when compiling an SSR project. If you run an SSR project locally, the errors don't show because the browser has access to the window object.

This was tested by

  • Creating a new nextjs project
  • Integrating Glean.js
  • Locally running next dev
  • Building locally via next build
  • Running the out directory on a simple web server via Python from my mac python3 -m http.server

Pull Request checklist

  • Quality: Make sure this PR builds and runs cleanly.
    • Inside the glean/ folder, run:
      • npm run test Runs all tests
      • npm run lint Runs all linters
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
  • Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work

@rosahbruno rosahbruno force-pushed the 1845362-ssr-support branch 2 times, most recently from 211fc7c to 0dfb438 Compare July 25, 2023 18:34
@rosahbruno rosahbruno force-pushed the 1845362-ssr-support branch from 0dfb438 to d2f801c Compare July 25, 2023 18:57
@rosahbruno rosahbruno marked this pull request as ready for review July 25, 2023 19:00
@auto-assign auto-assign bot requested a review from travis79 July 25, 2023 19:00
Copy link
Contributor

@chutten chutten left a comment

Choose a reason for hiding this comment

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

Normally I'd ask for instrumentation and a test, but it's likely a little difficult to instrument in a situation where instrumentation isn't possible and mocking window to be undefined might be tricky, too.

@rosahbruno rosahbruno merged commit 3486aea into mozilla:main Jul 26, 2023
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.

2 participants