unhandledRejection on remote import without body

[zaquest]

Hello.

Since the switch from requests to native-request UrlFileManager does not handle redericts anymore. This leads to an error here (and possibly in other similar code) if response body is empty (like often happens with 301 responses). The error is

Warning: Empty body (HTTP 301) returned by "https://url.that/redirects/here"

.../node_modules/less/lib/less/import-manager.js:97
                var contents = loadedFile.contents.replace(/^\uFEFF/, '');
                                                   ^

TypeError: Cannot read property 'replace' of null
    at loadFileCallback (.../node_modules/less/lib/less/import-manager.js:97:52)

Which might be fine when you use less from CLI. But the real issue happens when you try to call less.parse directly in your code. Somehow instead of passing the error to a callback, or rejecting a promise less.parse causes unhandledRejection.

[matthew-dean]

@zaquest Have a recommendation or alternate NPM library?

[zaquest]

I think a decision needs to be made about what happens if the response is empty. If the desired behaviour is to keep going, then resolving a promise with something like { contents: body || '', ...} here should suffice, or alternatively the promise should be rejected. Returning an empty string without attempting to follow the redirect feels weird to me though - it's even worse than simply rejecting a promise. Like, it's ok to not implement something, but it's not ok to implement something knowingly broken.

So I would either reject a promise in UrlFileManager.loadFile() with an error saying UrlFileManager doesn't support redirects, or would replace native-requests with something that knows how to follow redirects and then would return an empty string. But, honestly, rejecting in the second case is also fine, after all if there's no body after all the redirects then there's something wrong with the resource user pointed to.

Regarding other HTTP client libraries, axios seems to be on the raise.

[matthew-dean]

@zaquest This isn't a use-case I use, so either solution sounds good. Axios has a big footprint. The idea was to keep dependencies small. Following redirects makes sense to me, so maybe just something small that can handle that, and then sufficient tests (not sure how we'd test? A local HTTP server?) to cover this bug / issue.

Any PR along those lines would be welcome.