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

fix: allow amplitude to be imported/required during SSR #436

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

seanparmelee
Copy link
Contributor

Summary

We have a number of NextJS applications using amplitude, but we currently need to do something like the following to be able to use the library:

  useEffect(() => {
    import('amplitude-js').then((amplitude) => {
      amplitude.getInstance().init(API_KEY);
      amplitude.getInstance().logEvent('foo');
    });
  }, []);

If instead we were to have a static

import amplitude from 'amplitude-js'

at the top of our modules, we'd see errors due to amplitude trying to access global browser variables such as window and navigator when trying to load the module.

While I understand that this library is intended for use in the browser and there's a separate package for use in Node environments, our use case is to simply be able to import the module rather than having to use a dynamic import. We don't intend on calling any of the amplitude functions as part of the server-side render.

This PR adds the minimal amount of typeof checks in front of places where browser global variables were being accessed so that we could successfully import amplitude and build the app. We could go even further with this and add similar checks in all the places (i.e. in case functions like getInstance and logEvent were to be called during SSR) so that things fail more gracefully if you prefer.

Related: #138 #164

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: No

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.

Looks great - thank you!

@jooohhn jooohhn self-requested a review October 21, 2021 16:59
@seanparmelee
Copy link
Contributor Author

@ajhorst @qingzhuozhen @justin-fiedler saw you recently reviewed/merged some other PRs and was hoping you could help review this one. Thanks in advance!

Copy link
Contributor

@jooohhn jooohhn left a comment

Choose a reason for hiding this comment

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

Thanks for this @seanparmelee, I tested it on a Next app and LGTM

@jooohhn jooohhn merged commit ff7d8ef into amplitude:main Nov 2, 2021
github-actions bot pushed a commit that referenced this pull request Nov 2, 2021
## [8.9.1](v8.9.0...v8.9.1) (2021-11-02)

### Bug Fixes

* allow amplitude to be imported/required during SSR ([#436](#436)) ([ff7d8ef](ff7d8ef))
@seanparmelee seanparmelee deleted the ssr branch November 2, 2021 15:16
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.

3 participants