Closed IsaiahByDayah closed 5 months ago
Digging into how <BUILTIN_MODULES>
determines what the builtin module are, it looks to use the exported builtinModules
array from import { builtinModules } from "module"
in constants.ts
.
Following this open node issue, it seems like the node maintainers can not (or will not?) add "test"
to the exported builtinModules
array (the issue is still open so maybe it will be added one day?). Regardless, because the regex for determining builtin modules assumes the builtinModules
array contains all the actual builtin modules, "node:test"
is not captured in the regex and gets split from the other modules
Suggestion
My suggestion to fix would be to explicitly add a check for "node:test"
to BUILTIN_MODULES_REGEX_STR
, or at least allow for a way to capture modules that aren't a part of the exported builtinModules
array (as it seems that can be out of date / not all encompassing)
Though I think I've debugged the issue, I'm not much of a RegEx wizard so am not sure how best to fix the issue
Thanks for digging into this. I'm a bit reluctant to hack around an open issue in node (I don't see anything in the issue indicating that it shouldn't be fixed, at least). But if this is important to you and you're willing to give it a shot, I'll be happy to review a PR, so long as it doesn't add a ton of complexity and ideally is something we can revert in the future if/when node fixes the issue on their end. In this case I think it's not too bad. The regex we're creating is something like:
`^(?:node:)?(?:node:fs|node:path)$`
After the module names are joined together. So, after the join, you'd need to add node:test
. Maybe something like this:
export const BUILTIN_MODULES_REGEX_STR = `^(?:node:)?(?:${builtinModules.join(
'|',
)}${|node:test})$`;
If you try that (and maybe a comment as to why we need it), and add a test, and it works, I'd be happy to merge and release a new patch version.
Thanks again!
Cool, sounds good. Will try and get around to putting together a PR then. Thanks! 🙏🏾
Minor tweak, Instead of manually adding the |node:test
to the regex expression, I would create an array ahead of time, because the regex already dynamically matches if the node:
prefix is present, so something like this:
import { builtinModules } from 'module';
// Patched to add node:test due to https://github.com/nodejs/node/issues/42785
const patchedBuiltIns = […builtinModules, “test”];
@fbartho My only issue with that approach is that the regex would then also match against "test"
by itself, which isn't a valid builtin module (only the prefixed "node:test"
is, I'm guessing thats why its not already included in builtinModules
)
I just opened a PR to address it with a slight regex change. From my testing this seems to be backwards compatible (its basically "the original regex" OR node:test
) while also being pretty simple to "undo" should node ever change things and it becomes redundant
Nice catch @IsaiahByDayah — I didn’t realize “test”
wasn’t a valid module by itself.
No objections.
Your Environment
Describe the bug
Imports from
node:test
are not properly grouped with other builtin modulesTo Reproduce
Try to import from
node:test
and notice that it is not included with othernode:*
importsExpected behavior
imports from
node:test
would be grouped with the other builtin modulesScreenshots, code sample, etc
https://github.com/IanVS/prettier-plugin-sort-imports/assets/6565265/a7dee908-d49a-4e73-b461-37457c20b38c
Configuration File (cat .prettierrc, prettier.config.js, .prettier.js)
Error log
N/A
Contribute to @ianvs/prettier-plugin-sort-imports