Fix #3576 import redirects. Replace native-request with needle. (#3582)

* Fix #3576 import redirects. Replace native-request with needle. * Downgrade nock to support node 8
less · Jan 3, 2021 · ec74875 · ec74875
1 parent bfad962
commit ec74875
Show file tree

Hide file tree

Showing 7 changed files with 124 additions and 40 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/less/package-lock.json b/packages/less/package-lock.json
diff --git a/packages/less/package.json b/packages/less/package.json
@@ -50,7 +50,7 @@
 		"image-size": "~0.5.0",
 		"make-dir": "^2.1.0",
 		"mime": "^1.4.1",
-		"native-request": "^1.0.5",
+		"needle": "^2.5.2",
 		"source-map": "~0.6.0"
 	},
 	"devDependencies": {
@@ -82,6 +82,7 @@
 		"mocha": "^6.2.1",
 		"mocha-headless-chrome": "^2.0.3",
 		"mocha-teamcity-reporter": "^3.0.0",
+		"nock": "^11.8.2",
 		"npm-run-all": "^4.1.5",
 		"performance-now": "^0.2.0",
 		"phin": "^2.2.3",

diff --git a/packages/less/src/less-node/url-file-manager.js b/packages/less/src/less-node/url-file-manager.js
@@ -13,7 +13,7 @@ UrlFileManager.prototype = Object.assign(new AbstractFileManager(), {
     loadFile(filename, currentDirectory, options, environment) {
         return new Promise((fulfill, reject) => {
             if (request === undefined) {
-                try { request = require('native-request'); }
+                try { request = require('needle'); }
                 catch (e) { request = null; }
             }
             if (!request) {
@@ -22,23 +22,27 @@ UrlFileManager.prototype = Object.assign(new AbstractFileManager(), {
             }
 
             let urlStr = isUrlRe.test( filename ) ? filename : url.resolve(currentDirectory, filename);
-            
+
             /** native-request currently has a bug */
             const hackUrlStr = urlStr.indexOf('?') === -1 ? urlStr + '?' : urlStr
 
-            request.get(hackUrlStr, (error, body, status) => {
-                if (status === 404) {
-                    reject({ type: 'File', message: `resource '${urlStr}' was not found\n` });
+            request.get(hackUrlStr, { follow_max: 5 }, (err, resp, body) => {
+                if (err || resp.statusCode >= 400) {
+                    const message = resp.statusCode === 404
+                        ? `resource '${urlStr}' was not found\n`
+                        : `resource '${urlStr}' gave this Error:\n  ${err || resp.statusMessage || resp.statusCode}\n`;
+                    reject({ type: 'File', message });
                     return;
                 }
-                if (error) {
-                    reject({ type: 'File', message: `resource '${urlStr}' gave this Error:\n  ${error}\n` });
+                if (resp.statusCode >= 300) {
+                    reject({ type: 'File', message: `resource '${urlStr}' caused too many redirects` });
                     return;
                 }
+                body = body.toString('utf8');
                 if (!body) {
-                    logger.warn(`Warning: Empty body (HTTP ${status}) returned by "${urlStr}"`);
+                    logger.warn(`Warning: Empty body (HTTP ${resp.statusCode}) returned by "${urlStr}"`);
                 }
-                fulfill({ contents: body, filename: urlStr });
+                fulfill({ contents: body || '', filename: urlStr });
             });
         });
     }

diff --git a/packages/less/test/index.js b/packages/less/test/index.js
@@ -1,7 +1,8 @@
 var lessTest = require('./less-test'),
     lessTester = lessTest(),
     path = require('path'),
-    stylize = require('../lib/less-node/lessc-helper').stylize;
+    stylize = require('../lib/less-node/lessc-helper').stylize,
+    nock = require('nock');
 
 console.log('\n' + stylize('Less', 'underline') + '\n');
 
@@ -39,7 +40,7 @@ var testMap = [
     // TODO: Change this to rewriteUrls: false once the relativeUrls option is removed
     [{math: 'strict', relativeUrls: false, rootpath: 'folder (1)/'}, 'static-urls/'],
     [{math: 'strict', compress: true}, 'compression/'],
-    
+
     [{math: 0, strictUnits: true}, 'units/strict/'],
     [{math: 0, strictUnits: false}, 'units/no-strict/'],
 
@@ -88,3 +89,20 @@ lessTester.testSyncronous({syncImport: true}, 'math/strict/css');
 lessTester.testNoOptions();
 lessTester.testJSImport();
 lessTester.finished();
+
+(() => {
+    // Create new tester, since tests are not independent and tests
+    // above modify tester in a way that breaks remote imports.
+    lessTester = lessTest();
+    var scope = nock('https://example.com')
+        .get('/redirect.less').query(true)
+        .reply(301, null, { location: '/target.less' })
+        .get('/target.less').query(true)
+        .reply(200);
+    lessTester.runTestSet(
+        {},
+        'import-redirect/',
+        lessTester.testImportRedirect(scope)
+    );
+    lessTester.finished();
+})();
diff --git a/packages/less/test/less-test.js b/packages/less/test/less-test.js
@@ -1,5 +1,23 @@
 /* jshint latedef: nofunc */
 
+var logger = require('../lib/less/logger').default;
+
+var isVerbose = process.env.npm_config_loglevel !== 'concise';
+logger.addListener({
+    info(msg) {
+        if (isVerbose) {
+            process.stdout.write(msg + '\n');
+        }
+    },
+    warn(msg) {
+        process.stdout.write(msg + '\n');
+    },
+    erro(msg) {
+        process.stdout.write(msg + '\n');
+    }
+});
+
+
 module.exports = function() {
     var path = require('path'),
         fs = require('fs'),
@@ -14,8 +32,6 @@ module.exports = function() {
     var oneTestOnly = process.argv[2],
         isFinished = false;
 
-    var isVerbose = process.env.npm_config_loglevel !== 'concise';
-
     var testFolder = path.dirname(require.resolve('@less/test-data'));
     var lessFolder = path.join(testFolder, 'less');
 
@@ -27,20 +43,6 @@ module.exports = function() {
         }
     }
 
-    less.logger.addListener({
-        info: function(msg) {
-            if (isVerbose) {
-                process.stdout.write(msg + '\n');
-            }
-        },
-        warn: function(msg) {
-            process.stdout.write(msg + '\n');
-        },
-        error: function(msg) {
-            process.stdout.write(msg + '\n');
-        }
-    });
-
     var queueList = [],
         queueRunning = false;
     function queue(func) {
@@ -519,6 +521,23 @@ module.exports = function() {
         ok(stylize('OK\n', 'green'));
     }
 
+    function testImportRedirect(nockScope) {
+        return (name, err, css, doReplacements, sourcemap, baseFolder) => {
+            process.stdout.write('- ' + path.join(baseFolder, name) + ': ');
+            if (err) {
+                fail('FAIL: ' + (err && err.message));
+                return;
+            }
+            const expected = 'h1 {\n  color: red;\n}\n';
+            if (css !== expected) {
+                difference('FAIL', expected, css);
+                return;
+            }
+            nockScope.done();
+            ok('OK');
+        };
+    }
+
     return {
         runTestSet: runTestSet,
         runTestSetNormalOnly: runTestSetNormalOnly,
@@ -527,6 +546,7 @@ module.exports = function() {
         testSourcemap: testSourcemap,
         testSourcemapWithoutUrlAnnotation: testSourcemapWithoutUrlAnnotation,
         testImports: testImports,
+        testImportRedirect: testImportRedirect,
         testEmptySourcemap: testEmptySourcemap,
         testNoOptions: testNoOptions,
         testJSImport: testJSImport,

diff --git a/packages/test-data/less/import-redirect/import-redirect.less b/packages/test-data/less/import-redirect/import-redirect.less
@@ -0,0 +1,3 @@
+@import "https://example.com/redirect.less";
+
+h1 { color: red; }