| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
But is there a purpose behind Node’s internal resolve hook sometimes returning format: null as opposed to the key being missing? If not, maybe our internal hooks should always either set format if it has a truthy value, or don’t define it at all (or set format: undefined) and leave null out of it.
Sorry, something went wrong.
|
@nodejs/loaders |
Sorry, something went wrong.
I don't think there is a purpose of setting it to null rather than undefined, but IIRC @aduh95 added the change, so maybe he has a reason? That being said, in this case, the null vs undefined could be seen asa "was the format even checked?": |
Sorry, something went wrong.
|
Unless there’s a good reason we need null, I think we should first land this and then reopen #52988 and update it so that format is either a string or undefined (at least as set by Node internals).
I’m not sure that this is a good reason. If this is data that we want to preserve, it could be its own property on context, like formatWasChecked: true, and therefore it would be explicit and not mysterious. |
Sorry, something went wrong.
|
IIR, the historical reason (advocated by Antoine) was that null vs undefined was immaterial/unimportant and the historical precedent was nil. Splitting hairs: undefined means "idk" and null means "empty" (where empty is "i explicitly acknowledged this exists and there is nothing here" whereas undefined means 🤷♂️ or "ignore me") |
Sorry, something went wrong.
|
In this case, the different is very material—some tests fail with null but not with undefined. This is a footgun for hooks authors, to need to handle an unexpected extra potential return value. Unless we need null for some reason, I think we should toss it and stick to just strings and undefined. |
Sorry, something went wrong.
|
Just for sake of discussion, what value would be put in place for an unknown format module? (But maybe this is a discussion for another time) |
Sorry, something went wrong.
Without custom hooks registered, Node throws on unknown formats. |
Sorry, something went wrong.
There was a problem hiding this comment.
Changing the test should happen in the same commit that changes the code
Sorry, something went wrong.
|
You can use this PR to land the variable renaming if you want |
Sorry, something went wrong.
Unlike the other test changes for --experimental-detect-module, I think this one deserves its own PR, as IMO it should have been updated in #50314 to account for the new return value (but there was really no way to tell that this would happen). WDYT @GeoffreyBooth |
Sorry, something went wrong.
|
@redyetidev I think reopen #52988 and have it undo the part of #50314 that returned null instead of undefined; and update the tests accordingly. Unless @aduh95 can point to a reason that sometimes returning null is desirable, it seems to me that the returning of null was an unintended change in #50314 that had no documentation or explanation, and we should just undo that part. |
Sorry, something went wrong.
Sure, it'll take some work because it seems that certain infrastructure rely on that behavior, but I'll work on it |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Pull request #50314 added support for a null format value in addition to the existing commonjs or module. However, the test for ESM hooks relied on the default value of commonjs, causing it to fail when run with the --experimental-detect-module flag. This PR enables the test to detect null formats, ensuring it runs successfully with experimental detection enabled.
Sidenote: While @GeoffreyBooth and I were debugging this behavior, the single-character function arguments made it very difficult to understand, so this PR also expands them into the full names.