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

Replace global with window and ensure its existence #3990

Merged
merged 1 commit into from Jul 11, 2023

Conversation

ivarnakken
Copy link
Member

Description

addEventListener and removeEventListener are methods provided by the window object in the browser, but not on the global object in a Node environment. This was causing a TypeError.

Testing

  • I have thoroughly tested my changes.

The fancy nodes still work.


Resolves webkom/lego#3434

`addEventListener` and `removeEventListener` are methods
provided by the `window` object in the browser, but not on the `global`
object in a Node environment. This was causing a TypeError.
@ivarnakken ivarnakken added types Pull requests that improve or fix types review-needed Pull requests that need review bug-fix Pull requests that fix a bug labels Jun 11, 2023
@ivarnakken ivarnakken requested a review from eikhr June 11, 2023 09:59
@ivarnakken ivarnakken self-assigned this Jun 11, 2023
Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

You're not wrong, but I think it was working fine with global. The Sentry errors was me testing some stuff locally. I believe global === window in the browser.

Have you tested this with SSR? I remember last time I was working on FancyNodesCanvas it was a bit tricky getting it to render the nodes initially (before resizing) with SSR.

@ivarnakken
Copy link
Member Author

The Sentry errors was me testing some stuff locally.

I recently deployed to staging, so I just assumed it was my fault hehe.

Have you tested this with SSR? I remember last time I was working on FancyNodesCanvas it was a bit tricky getting it to render the nodes initially (before resizing) with SSR.

Tested with SSR now, and it seems to work fine.

@ivarnakken
Copy link
Member Author

@eikhr I guess there's little point in merging this in without the migration to vite. Any updates on that?

@ivarnakken ivarnakken added approved Pull requests that have been approved do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged and removed review-needed Pull requests that need review labels Jun 28, 2023
@eikhr
Copy link
Member

eikhr commented Jul 11, 2023

No updates on vite, it will require a fair bit of css refactoring to make stuff work. It's in my backlog, but not quite at the top.
I don't see any problems with merging this though. It will just be one less problem to deal with whenever we migrate to vite.

@ivarnakken ivarnakken added ready-to-merge Pull requests that have been approved and are ready to be merged and removed do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged labels Jul 11, 2023
@ivarnakken
Copy link
Member Author

It will just be one less problem to deal with whenever we migrate to vite.

Fair enough

@ivarnakken ivarnakken merged commit 2fcf04c into master Jul 11, 2023
4 checks passed
@ivarnakken ivarnakken deleted the fix-type-error-on-fancy-nodes branch July 11, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved bug-fix Pull requests that fix a bug ready-to-merge Pull requests that have been approved and are ready to be merged types Pull requests that improve or fix types
Projects
None yet
2 participants