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

use globalThis instead of window in HMR #134

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MaxArtemov
Copy link

@MaxArtemov MaxArtemov commented Jul 24, 2023

Usage of window in non-browser env, breaks

@antoniopresto
Copy link

antoniopresto commented Jul 28, 2023

+ 1

This fixes window is not defined in next.js "edge runtimes"

for more context: https://wintercg.org/

The ultimate goal of the group is to promote runtimes supporting a comprehensive unified API surface that JavaScript developers can rely on regardless of the runtime they are using: be it browsers, servers, embedded applications, or edge runtimes. The members of the group want to provide a space to better coordinate between browser vendors and other implementors on how Web Platform APIs can be best implemented and used outside of browsers.

src/hooks/useHMR.ts Outdated Show resolved Hide resolved
@afc163 afc163 requested a review from zombieJ July 29, 2023 11:21
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2023

Codecov Report

Patch coverage: 87.17% and project coverage change: -0.06% ⚠️

Comparison is base (55d5716) 94.80% compared to head (89f350b) 94.75%.
Report is 10 commits behind head on master.

❗ Current head 89f350b differs from pull request most recent head 430abbf. Consider uploading reports for the commit 430abbf to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
- Coverage   94.80%   94.75%   -0.06%     
==========================================
  Files          24       24              
  Lines        2041     2115      +74     
  Branches      321      330       +9     
==========================================
+ Hits         1935     2004      +69     
- Misses        106      111       +5     
Files Changed Coverage Δ
src/hooks/useHMR.ts 52.94% <33.33%> (-1.61%) ⬇️
src/hooks/useGlobalCache.tsx 93.93% <78.57%> (-2.62%) ⬇️
src/hooks/useStyleRegister/index.tsx 98.11% <87.34%> (-0.03%) ⬇️
src/hooks/useCacheToken.tsx 99.44% <100.00%> (+0.04%) ⬆️
src/util.ts 40.54% <100.00%> (+1.65%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: antoniopresto <[email protected]>
@XavierLeTohic
Copy link

+1 tested quickly via this loader and it fixed window is not defined using renderToPipeableStream from react-dom/server with Express

{
  test: /node_modules[\\/]@ant-design[\\/]cssinjs[\\/]es[\\/]hooks[\\/]useHMR\.js$/, // https://github.com/ant-design/cssinjs/issues/133
  loader: "string-replace-loader",
  options: {
	  search: "window",
	  replace: "globalThis",
	  flags: "g",
  },
},

@antoniopresto
Copy link

antoniopresto commented Aug 13, 2023

Hi! Do you need any help on this? :)

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.

5 participants