Page MenuHomePhabricator

grunt-stylelint 0.20.1 fails when jsdom-global runs in same process
Closed, ResolvedPublic

Description

This is some very strange interaction between different things. Steps to reproduce:

Stylelint will fail:

Running "stylelint:all" (stylelint) task                                                                                                                                                      
Warning: Invalid URL: lib/utils/FileCache.cjs Use --force to continue.                                                                                                                        
                                                                                                                                                                                              
Aborted due to warnings.

You can further narrow this down to see that the jsdom helper specifically is the culprit:

diff --git a/Gruntfile.js b/Gruntfile.js
index 4d6cd1da81..4f39140801 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -54,6 +54,5 @@ module.exports = function ( grunt ) {
 				specs: [
-					'tests/jasmine/**/*.spec.js'
 				],
 				helpers: [
-					'tests/jasmine/helpers/*.js'
+					'tests/jasmine/helpers/jsdom.js'
 				]

Specifically, the line require( 'jsdom-global' )(); in there is the problem (if you comment it out, Stylelint works again).

Unfortunately, just commenting out the jsdom-global line also makes the Jasmine tests fail, so we need to find a better solution for this.

Event Timeline

It’s probably due to this line in node_modules/stylelint/lib/utils/FileCache.cjs:

const pkg = JSON.parse(node_fs.readFileSync(new URL('../../package.json', (typeof document === 'undefined' ? require('u' + 'rl').pathToFileURL(__filename).href : (_documentCurrentScript && _documentCurrentScript.src || new URL('lib/utils/FileCache.cjs', document.baseURI).href))), 'utf8'));

It looks like Stylelint is trying to detect whether it’s in a browser or not (typeof document === 'undefined')…

Bah – jsdom-global provides a “cleanup” function, but even if you use that, you just get a different error:

diff --git a/Gruntfile.js b/Gruntfile.js
index 4d6cd1da81..64b22ccc25 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -61,7 +61,10 @@ module.exports = function ( grunt ) {
 		}
 	} );
 
-	grunt.registerTask( 'test', [ 'eslint:all', 'banana', 'jasmine_nodejs', 'stylelint' ] );
+	grunt.registerTask( 'jasmine_nodejs_cleanup', 'Clean up state left behind by Jasmine', function () {
+		global.resetJsdom();
+	} );
+	grunt.registerTask( 'test', [ 'eslint:all', 'banana', 'jasmine_nodejs', 'jasmine_nodejs_cleanup', 'stylelint' ] );
 	grunt.registerTask( 'fix', function () {
 		grunt.config.set( 'eslint.options.fix', true );
 		grunt.task.run( 'eslint' );
diff --git a/tests/jasmine/helpers/jsdom.js b/tests/jasmine/helpers/jsdom.js
index 0c2210f62d..d9af1c776b 100644
--- a/tests/jasmine/helpers/jsdom.js
+++ b/tests/jasmine/helpers/jsdom.js
@@ -1,5 +1,5 @@
 // gives us a document to work on
-require( 'jsdom-global' )();
+global.resetJsdom = require( 'jsdom-global' )();
 // expected to exist by Vue (for instanceof check) but not provided by our jsdom version
 // eslint-disable-next-line no-restricted-globals
 global.SVGElement = function SVGElement() {};
Running "stylelint:all" (stylelint) task
Warning: URL is not defined Use --force to continue.

Because it just deletes global.URL instead of restoring Node’s original. I guess this is where it starts to become a problem that jsdom-global was last updated 7 years ago…

Well, applying this patch to jsdom-global (and still including the above hack to run the cleanup at all) fixes the issue:

diff --git a/index.js b/index.js
index 7827bd5480..54eb06ba64 100644
--- a/index.js
+++ b/index.js
@@ -28,8 +28,10 @@ module.exports = function globalJsdom (html, options) {
   var jsdom = require('jsdom')
   var document = new jsdom.JSDOM(html, options)
   var window = document.window
+  var originals = {}
 
   KEYS.forEach(function (key) {
+    originals[key] = global[key]
     global[key] = window[key]
   })
 
@@ -39,7 +41,7 @@ module.exports = function globalJsdom (html, options) {
   document.destroy = cleanup
 
   function cleanup () {
-    KEYS.forEach(function (key) { delete global[key] })
+    KEYS.forEach(function (key) { global[key] = originals[key] })
   }
 
   return cleanup

(Note that this doesn’t distinguish between “original was set to undefined” and “original was not set at all”. You could track this in a second variable, of course.)

But I don’t know if we want to fork the library for that… :/

I wonder if it wouldn’t be easier to ditch jsdom-global, and instead have the tests create a new JSDom document and assign global.window = document.window, and make all the non-test code access e.g. window.document instead of document. That way we only have one global to worry about, and in the browser it should work the same way (since window is the global there).

Vue might need a few more globals set (we’re already having to set SVGElement) but hopefully not too many… I think I’ll try out this approach.

Bleh, Vue reads document as soon as it initializes:

const doc = typeof document !== "undefined" ? document : null;

So we can’t do this in a beforeAll / afterAll, it’ll have to be a Jasmine helper again. And that probably means we’ll have to do the cleanup with a separate Grunt script as shown above, too.

But I think this approach (our own little jsdom-global, which only sets a very limited set of globals and also unsets them more carefully) could still be the way to go.

Change #1047100 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Replace jsdom-global with homegrown solution

https://gerrit.wikimedia.org/r/1047100

But I think this approach (our own little jsdom-global, which only sets a very limited set of globals and also unsets them more carefully) could still be the way to go.

Yup, seems to work well enough.

Change #1047038 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Update Stylelint dependencies

https://gerrit.wikimedia.org/r/1047038

Change #1047100 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Replace jsdom-global with homegrown solution

https://gerrit.wikimedia.org/r/1047100

Change #1047038 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Update Stylelint dependencies

https://gerrit.wikimedia.org/r/1047038