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

withSafeTimeout: Unguarded window reference breaks SSR #9265

Closed
ockham opened this issue Aug 23, 2018 · 5 comments
Closed

withSafeTimeout: Unguarded window reference breaks SSR #9265

ockham opened this issue Aug 23, 2018 · 5 comments
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality

Comments

@ockham
Copy link
Contributor

ockham commented Aug 23, 2018

Describe the bug
See title, and Automattic/wp-calypso#26838 (comment). Trying to boot that Calypso branch (which uses @wp/compose results in

~/src/wp-calypso/build/webpack:/node_modules/@wordpress/compose/build-module/with-safe-timeout/index.js:29
var _window = window,
^
ReferenceError: window is not defined

To Reproduce
Steps to reproduce the behavior:

  1. Clone Calypso, and check out the branch for SDK: Try wp.data with calypso state tree Automattic/wp-calypso#26838
  2. Apply https://github.com/WordPress/gutenberg/pull/9261/files#r212224119 locally (to work around another issue that would otherwise get in the way)
  3. npm start

Expected behavior
Calypso booting

Additional context
Can we simply drop this line? Since setTimeout and clearTimeout are methods of the window object, they should be globally available -- as they are in Node.

@tofumatt
Copy link
Member

I thought we could reference those directly but maybe I'm wrong? They're not language functions though, right? They're part of window… or really part of WindowOrWorkerGlobalScope

@tofumatt tofumatt added [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality labels Aug 23, 2018
@ockham
Copy link
Contributor Author

ockham commented Aug 23, 2018

I thought we could reference those directly but maybe I'm wrong? They're not language functions though, right? They're part of window… or really part of WindowOrWorkerGlobalScope

Uh, not sure I'm getting you right. Yeah, so I think they are part of window (for all practical purposes 😉) and should thus be available globally. Same on the node side, hence my proposed fix in #9266...

@tofumatt
Copy link
Member

Ah, I just wasn't sure how pure we were being about those globals; elsewhere I see "globals" being taken off window too; I wondered if there was a reason we were doing it 🤷‍♂️

@aduth
Copy link
Member

aduth commented Aug 23, 2018

These are globals and we shouldn't be referencing them off the window object.

I'm recalling some previous discussion with @iseulde on this, so... cc 😄

@aduth
Copy link
Member

aduth commented Aug 23, 2018

Previously: #2271 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants