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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jan 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddec00d
Fix #3576 import redirects. Replace native-request with needle.
zaquest Dec 29, 2020
fc2c9ca
Downgrade nock to support node 8
zaquest Dec 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

60 changes: 49 additions & 11 deletions packages/less/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/less/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down Expand Up @@ -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",
Expand Down
22 changes: 13 additions & 9 deletions packages/less/src/less-node/url-file-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 });
});
});
}
Expand Down
22 changes: 20 additions & 2 deletions packages/less/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');

Expand Down Expand Up @@ -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/'],

Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If remote imports are being broken between tests, where do you think would be the best place to fix this? Is this a problem in Less itself? Or a problem in the way tests are set up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is in the way tests are set up. Ideally tests should be independent, meaning each test should have it's own instance of less, even more so since the less instance is modified by some tests. Or, alternatively, each test should have a tear down part, where it returns less instance to it's initial (before test) state. However, if I understand correctly tests are also run concurrently, which might be a problem for the set up / tear down approach.

I suspect the reason remote imports get broken is because of plugin tests, might be wrong here, haven't actually investigated it.

One way to approach this is to define a function with contents of less-node/index.js and call it in less-node/index.js and before each test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe tests are run concurrently. They're run in a queue. But definitely it's possible they mutate the less object or don't return it to previous state. However, I doubt that's intentional, so it would be great to track it down, but maybe that's out of the scope of this issue.

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();
})();
52 changes: 36 additions & 16 deletions packages/less/test/less-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'),
Expand All @@ -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');

Expand All @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -527,6 +546,7 @@ module.exports = function() {
testSourcemap: testSourcemap,
testSourcemapWithoutUrlAnnotation: testSourcemapWithoutUrlAnnotation,
testImports: testImports,
testImportRedirect: testImportRedirect,
testEmptySourcemap: testEmptySourcemap,
testNoOptions: testNoOptions,
testJSImport: testJSImport,
Expand Down
3 changes: 3 additions & 0 deletions packages/test-data/less/import-redirect/import-redirect.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
@import "https://example.com/redirect.less";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that will always redirect? As in, will this test always succeed with this remote endpoint?

Copy link
Contributor Author

@zaquest zaquest Jan 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the url nock mocks, so node doesn't actually send the request to a remote server. So as long as nock is set up to intercept this exact url it will always redirect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, perfect.


h1 { color: red; }