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

ReferenceError: self is not defined ( Mocha+JSdom ) #177

Closed
dlin-me opened this Issue Feb 27, 2017 · 14 comments

Comments

Projects
None yet
6 participants
@dlin-me

dlin-me commented Feb 27, 2017

Version

0.13.2

Request

Bug

Current behavior

Got the following error when running test cases with mocha+jsdom. Same test run fine with version 0.13.1

ReferenceError: self is not defined
    at eval (Users/dlin/Projects/insiight/onboarding/node_modules/style-loader/addStyles.js:14:30)
    at eval (Users/dlin/Projects/insiight/onboarding/node_modules/style-loader/addStyles.js:9:47)
    at module.exports (Users/dlin/Projects/insiight/onboarding/node_modules/style-loader/addStyles.js:31:68)
@ekulabuhov

This comment has been minimized.

Member

ekulabuhov commented Mar 2, 2017

Hi! Do you mind providing a bare bone repro of your issue?

@frodare

This comment has been minimized.

frodare commented Mar 2, 2017

This patch broke our testing build. After looking into the problem we were able to fix it by defining a global self variable in our jsdom setup like this:

var jsdom = require('jsdom');
global.document = jsdom.jsdom('<!DOCTYPE html><head></head><html><body></body></html>');
global.window = document.defaultView;
global.window.document = global.document;
// fix
global.self = global.window;
@ekulabuhov

This comment has been minimized.

Member

ekulabuhov commented Mar 2, 2017

This issue was caused by 9dc45a6.

The problem here is that global.self is only defined in Browsers and Web Workers. Unit tests running in NodeJs environment won't have access to global.self.

@FlyingDR

This comment has been minimized.

Contributor

FlyingDR commented Mar 5, 2017

Same issue breaks attempts to use style-loader to build Firefox add-ons (at least SDK-based) since self here contains no navigator property and it results into TypeError: self.navigator is undefined error

@ekulabuhov

This comment has been minimized.

Member

ekulabuhov commented Mar 6, 2017

style-loader is designed to be used in the browser only. Both self and navigator are standard properties of HTML5 spec. While it is possible to make style-loader work in other environments, it is not something that is supported or guaranteed to work.

@FlyingDR

This comment has been minimized.

Contributor

FlyingDR commented Mar 6, 2017

@ekulabuhov Since the only barrier for use of style-loader into other environments is a test for old IE browsers - what stops this test from returning false for these non-standard environments which are not old IE by itself?

Updating this line of code with something like
return window && document && document.all && !window.atob;

may solve the problem and still be able to detect IE 6-9. Test itself is taken from Browserhacks.

@ekulabuhov

This comment has been minimized.

Member

ekulabuhov commented Mar 6, 2017

@FlyingDR There's no problem with fixing this line. The problem is that there's no guarantee that the next feature/bug-fix will not break it again. Best advice here is to make your environment match the Browser API as much as possible.

@FlyingDR

This comment has been minimized.

Contributor

FlyingDR commented Mar 6, 2017

It is good point, but since there is real use cases when current code breaks builds - I think it is good idea to either update it to be more safe or at least to add some runtime error in development environment or maybe FAQ question so people that will not need to spend much time trying to find out why their build get broken by simple CSS attachment.

@ekulabuhov

This comment has been minimized.

Member

ekulabuhov commented Mar 6, 2017

I agree. And it should throw a runtime error as implemented here: 0d5089b. But it seems like some test environments are able to bypass that check. Meaning that they simulate some parts of the browser API while leaving out others.

@michael-ciniawsky

This comment has been minimized.

Member

michael-ciniawsky commented Mar 15, 2017

@FlyingDR Mind sending a PR with eventual fixes and to better discuss the issue? 😛

@FlyingDR

This comment has been minimized.

Contributor

FlyingDR commented Mar 16, 2017

@michael-ciniawsky I'm not quite sure if PR with solution proposed by me above will be acceptable, taking in mind obviously correct comments from @ekulabuhov

Personally I still think that it is quite safe to check for property existence before accessing them to make assumption about environment, code is running in, but this point may not be accepted by this project, I'm not part of. I can, of course, prepare PR based on my version of the fix.

@michael-ciniawsky

This comment has been minimized.

Member

michael-ciniawsky commented Mar 21, 2017

@FlyingDR Well I'm not sure about it 🙃 , seems it happend here 9dc45a6 if you not pissed of enterily if the PR gets rejected, send one please for futher discussion, otherwise I fully understand if you're not interested at all 😛

⚠️ It's definitely agreed upon that style-loader should only work in browsers enviroments (no SSR-ish]) behaviour

@FlyingDR

This comment has been minimized.

Contributor

FlyingDR commented Mar 21, 2017

@michael-ciniawsky I've added pull request #196

@d3viant0ne

This comment has been minimized.

Member

d3viant0ne commented Mar 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment