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

esm: throw on any non-2xx response #43742

Merged
merged 1 commit into from
Jul 12, 2022
Merged

esm: throw on any non-2xx response #43742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion lib/internal/modules/esm/fetch_module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ function fetchWithRedirects(parsed) {
err.message = `Cannot find module '${parsed.href}', HTTP 404`;
throw err;
}
if (res.statusCode < 200 || res.statusCode >= 400) {
// This condition catches all unsupported status codes, including
// 3xx redirection codes without `Location` HTTP header.
if (res.statusCode < 200 || res.statusCode >= 300) {
Copy link
Member

Choose a reason for hiding this comment

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

I think an explanation for this if condition would be helpful. Is this the correct reason?

Suggested change
if (res.statusCode < 200 || res.statusCode >= 300) {
// The 3xx codes that we support were handled above in isRedirect
// This condition catches all unsupported status codes
if (res.statusCode < 200 || res.statusCode >= 300) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment via 640b85d.
I think, this condition can be read properly and unambiguously as "we need 2xx code", so there is no difference between 1xx, 3xx, 5xx or any invalid number.
What we might need to point out is that isRedirect check above handled only machine-readable redirects with Location, so we want to filter them again. If this part will be refactored into some validateStatusCode that throws meaningful errors for different coderanges, we don't want these to be omitted.

throw new ERR_NETWORK_IMPORT_DISALLOWED(
Comment on lines +153 to 154
Copy link
Member

Choose a reason for hiding this comment

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

Are you thinking correcting the error be handled separately/subsequently?

I think it would be better to create helpers for these, like Kidonng just added with isRedirect(). Something like

if (!isStatusOk(res.statsuCode)) { // keeping with fetch's response.ok
  throw new ERR_NETWORK_IMPORT_BAD_RESPONSE(…);

I think response.ok is a bad prop name though because 200 = OK, and response.ok is any 2xx. If we're okay deviating, I think isStatusGood is a better name.

If you'd rather that also be a follow-up, sure, I can do.

A little troubling that no test needed to be updated for this 😨

Copy link
Contributor

@aduh95 aduh95 Jul 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think response.ok is a bad prop name though because 200 = OK, and response.ok is any 2xx. If we're okay deviating, I think isStatusGood is a better name.

Why deviate? AFAICT web browsers would accept any response with a 2xx status code as valid (as long as it has the correct Content-Type header), I think Node.js should too.

Repro 
// server.cjs
const http = require('node:http');
http.createServer((req, res) => {
  if (/^\/\d+$/.test(req.url))
    res.writeHead(Number(req.url.slice(1)), { 'Content-Type': 'text/javascript' });
  res.end();
}).listen(8000);

Then open http://localhost:8000 on the web browser you want to test and paste the following in the DevTools JS console:

Promise.allSettled(Array.from({length: 99}, (_, i) => import(`/${i+200}`))).then(console.log)

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean what reason would we have for deviating, or why would using a different name be deviating?

I'm not proposing different status codes, merely word-smithing the name of the potential helper (isStatusOk vs isStatusGood).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I don't understand what you mean by "because 200 = OK, and response.ok is any 2xx", or how that's a justification for preferring isStatusGood – if anything that seems like a reason for keeping ok in the helper name.
Anyway, I don't have any strong feeling regarding the name of the helper, I interpreted your comment as you wanted to limit valid response to code 200, as long as it's not the case I'm happy either way.

Copy link
Member

Choose a reason for hiding this comment

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

I interpreted your comment as you wanted to limit valid response to code 200

Noooo ;)

I don't understand what you mean by "because 200 = OK, and response.ok is any 2xx"

Fetch's response.ok is true when the status code is 2xx. However, status code 200's textual name is OK. I'm wondering if people might misunderstand isStatusOk to mean "is the status specifically 200 OK". isStatusGood removes the ambiguity but deviates from fetch's poorly named response.ok.

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'll be happy with any outcome that will be most appropriate according to "big picture" of this subsystem.

My personal preference for this part is to see it rewritten using fetch (i.e. undici). In that case, you can use response.ok directly and handle redirects without deviations from common ways. Additionally, you won't need to maintain this logic separately, e.g. if there will be new RFC with new codes.

As for naming and helper functions, my personal preference for 2xx codes is to have it inlined with hardcoded numbers as long as there is only one place where it's checked; or relying on response.ok if possible.
Same for 3xx if manual redirection handling will be kept: I think, having a function named isRedirect() or variable named isRedirect is exactly as self-explanatory in the code as needed.

Regarding isStatus* name, I agree that response.ok is poorly named. But if it's used on actual Response, it's ok. :)
isStatusGood sounds worse to me as it implies that there was some specific validation.
I'd suggest isStatusSuccess or isStatusSuccessful because it's canonical term for 2xx codes.

Copy link
Member

Choose a reason for hiding this comment

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

Oow, actually, that's an excellent point. Is there any reason we can't port this to actual fetch? Would we run into any cache contamination? 🤔 CORS would suddenly come into the picture.

Fetch didn't exist yet in Node when this was written, so that discussion hasn't already happened AFAIK.

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, porting this to fetch is feasible and desirable. CORS shouldn't be an issue for client-side Node.js.
Cache shouldn't be a problem as well: I don't know if ignoring cache-related HTTP headers was intentional, but Request.cache supports different modes, so you can either rely on it or handle manually.

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 know if ignoring cache-related HTTP headers was intentional

I vaguely remember that it was but I couldn't tell you the rationale at the time.

res.headers.location,
parsed.href,
Expand Down