Skip to main content

Bug Tracker

Side navigation

#13760 closed bug (fixed)

Opened April 10, 2013 01:08AM UTC

Closed April 10, 2013 09:27PM UTC

getComputedStyle no longer works in node with jsdom

Reported by: m_gol Owned by:
Priority: high Milestone: None
Component: build Version: 2.0.0-beta3
Keywords: Cc:
Blocked by: Blocking:
Description

I've noticed in this pull request:

https://github.com/jquery/jquery/pull/1103

a change to the parameter passed to the main enclosing IIFE, i.e. window was changed to this. The problem is, there are lots of places in jQuery where we assume window is actually window so it breaks a lot. One example is in css.js:

// NOTE: we've included the "window" in window.getComputedStyle
// because jsdom on node.js will break without it.
function getStyles( elem ) {
	return window.getComputedStyle( elem, null );
}

So we have a few bytes (window.) that actually fix nothing. Another example is window.addEventListener etc.

Attachments (0)
Change History (7)

Changed April 10, 2013 01:09AM UTC by m_gol comment:1

I meant "a few bytes that fix nothing", not "save".

Changed April 10, 2013 03:13AM UTC by rwaldron comment:2

_comment0: What a nightmare.1365563784836750

What a nightmare. There are way too many places where we need window to be _window_

Changed April 10, 2013 10:21AM UTC by m_gol comment:3

Dave suggested we could just pass window instead of this and thus attach jQuery to window. jQuery is exported for node separately, anyway, so it shouldn't break.

If we need the global object inside, too, though, I see no other option than

(function ( global, window ) {
    /* code */
})( this, window );

Changed April 10, 2013 03:02PM UTC by timmywil comment:4

component: unfiledbuild
priority: undecidedhigh
status: newopen

If I understand this correctly, jQuery won't work without jsdom and its window export, which means using node's global object isn't sufficient on its own. So, just passing window and continuing to assign to module.exports in the outro is probably the best option.

Changed April 10, 2013 04:30PM UTC by m_gol comment:5

description: I've noticed in this pull request:[[BR]] \ https://github.com/jquery/jquery/pull/1103 [[BR]] \ a change to the parameter passed to the main enclosing IIFE, i.e. window was changed to this. The problem is, there are lots of places in jQuery where we assume window is actually window so it breaks a lot. One example is in css.js: \ \ {{{ \ // NOTE: we've included the "window" in window.getComputedStyle \ // because jsdom on node.js will break without it. \ function getStyles( elem ) { \ return window.getComputedStyle( elem, null ); \ } \ }}} \ \ So we have a few bytes (`window.`) that actually save nothing. Another example is window.addEventListener etc.I've noticed in this pull request:[[BR]] \ https://github.com/jquery/jquery/pull/1103 [[BR]] \ a change to the parameter passed to the main enclosing IIFE, i.e. window was changed to this. The problem is, there are lots of places in jQuery where we assume window is actually window so it breaks a lot. One example is in css.js: \ \ {{{ \ // NOTE: we've included the "window" in window.getComputedStyle \ // because jsdom on node.js will break without it. \ function getStyles( elem ) { \ return window.getComputedStyle( elem, null ); \ } \ }}} \ \ So we have a few bytes (`window.`) that actually fix nothing. Another example is window.addEventListener etc.

Changed April 10, 2013 06:34PM UTC by m_gol comment:6

It's actually not src/outro, it's in src/exports.

Pull request: https://github.com/jquery/jquery/pull/1238

Changed April 10, 2013 09:27PM UTC by Michał Gołębiowski comment:7

resolution: → fixed
status: openclosed

Fixes #13760. Pass window, not this to the main IIFE. Closes gh-1238

Changeset: e80117adb49eff8f82140b01d755bdeefe7eb089