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.

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

node:test is not detected as <BUILTIN_MODULES> #142

Closed
1 task done
IsaiahByDayah opened this issue Jan 30, 2024 · 6 comments · Fixed by #143
Closed
1 task done

node:test is not detected as <BUILTIN_MODULES> #142

IsaiahByDayah opened this issue Jan 30, 2024 · 6 comments · Fixed by #143
Assignees

Comments

@IsaiahByDayah
Copy link
Contributor

IsaiahByDayah commented Jan 30, 2024

Your Environment

  • Prettier version: 3.0.3
  • node version: 20.11.0
  • package manager: pnpm@8
  • IDE: VScode

Describe the bug

Imports from node:test are not properly grouped with other builtin modules

To Reproduce

Try to import from node:test and notice that it is not included with other node:* imports

Expected behavior

imports from node:test would be grouped with the other builtin modules

Screenshots, code sample, etc

Screen.Recording.2024-01-29.at.11.35.45.PM.mov

Configuration File (cat .prettierrc, prettier.config.js, .prettier.js)

importOrder: [
    "",
    "<BUILTIN_MODULES>", // Node.js built-in modules
    "",
    "<THIRD_PARTY_MODULES>", // Imports not matched by other special words or groups.
    "",
    "^[.]", // relative imports
  ],

Error log

N/A

Contribute to @ianvs/prettier-plugin-sort-imports

  • I'm willing to fix this bug 🥇
@IsaiahByDayah
Copy link
Contributor Author

IsaiahByDayah commented Jan 30, 2024

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

@IanVS
Copy link
Owner

IanVS commented Jan 30, 2024

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!

@IsaiahByDayah
Copy link
Contributor Author

Cool, sounds good. Will try and get around to putting together a PR then. Thanks! 🙏🏾

@fbartho
Copy link
Collaborator

fbartho commented Jan 31, 2024

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”];

@IsaiahByDayah
Copy link
Contributor Author

@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

@fbartho
Copy link
Collaborator

fbartho commented Jan 31, 2024

Nice catch @IsaiahByDayah — I didn’t realize “test” wasn’t a valid module by itself.

No objections.

IanVS pushed a commit that referenced this issue Jan 31, 2024
Following discussion in #142 , this adds an explicit check for
`"node:test"` to `BUILTIN_MODULES_REGEX_STR` in order to properly group
it with other builtin modules.

Test covers:
- `"node:test"` correctly gets grouped in with other builtin node
modules
- `"node:test"` is correctly sorted among other builtin node modules
- `"*node:test"` is NOT grouped in with builtin modules
- `"node:test*"` is NOT grouped in with builtin modules
- `"node"` is NOT grouped in with builtin modules
- `"test"` is NOT grouped in with builtin modules

Fixes #142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants