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

guard all window references #1318

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

lindapaiste
Copy link
Contributor

In order to run in Node.js we must guard all window references with typeof window !== 'undefined' before accessing the window variable. Otherwise it throws a fatal error ReferenceError: window is not defined as reported in issue #1317.

This PR addresses all uses of window in the /src directory:

  • Copies some of the code from my pending PR Code clean-up: class-methods-use-this #1312 which guards window.location.pathname in the modelLoader utility and removes it from the FaceApi class.
  • Accesses Color class through p5Utils.p5Instance.Color instead window.p5.Color in BodyPix. (Note: this was previously guarded by p5Utils.checkP5, so it would only cause an error if the user was using p5 in Node).
  • Adds a guard around window in the P5Util constructor. There was already a guard in the P5Util.p5Instance getter, but I believe that is too late as the error will have already been thrown in the constructor.

There will be additional changes needed to run in Node.js, such as guarding classes HTMLImageElement, etc.

@joeyklee joeyklee added the SEMVER/patch a version tag for a patch release change label Mar 7, 2022
Copy link
Contributor

@joeyklee joeyklee left a comment

Choose a reason for hiding this comment

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

Having reviewed #1319 -- I'm going to give this one a 👍 !! Thanks so much for these incremental PRs ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SEMVER/patch a version tag for a patch release change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants