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

More strict on what is considered a builtin - Fixes #16 #17

Merged
merged 5 commits into from
May 18, 2022
Merged

More strict on what is considered a builtin - Fixes #16 #17

merged 5 commits into from
May 18, 2022

Conversation

fbartho
Copy link
Collaborator

@fbartho fbartho commented May 17, 2022

Relates to #16

Looks like a combination of config options are conflicting?

Not sure why it triggers on CLI but not in IDE for me, but the run_spec is able to reproduce it in this repo.

@fbartho
Test case for issue 16
00148f4 
Relates to IanVS#16

Looks like a combination of config options are conflicting?

Not sure why it triggers on CLI vs API for me, but the run_spec is able to reproduce it in this repo.
@fbartho fbartho mentioned this pull request May 17, 2022
Closed
1 task
@IanVS
Copy link
Owner

IanVS commented May 17, 2022

@fbartho I've invited you as a collaborator, so you can make branches without a fork, to make it easier to collaborate. I'm taking a look into this now. I updated my project at work to the latest version, and running prettier in the cli does not throw any errors, so at least it doesn't seem like something that broke in the latest release.

@IanVS
Copy link
Owner

IanVS commented May 17, 2022

This appears to be related to importOrderBuiltinModulesToTop. Setting that to false makes this test case pass.

@IanVS
Copy link
Owner

IanVS commented May 17, 2022

constants is one of the builtin node modules. The regex needs to be more explicit. I should be able to crank out a fix, thanks for pointing this out!

@blutorange
Copy link
Contributor

blutorange commented May 17, 2022 •
edited
Loading

How about changing this to

`^(${builtinModules.join("|")}|${builtinModules.map(module => 'node:'+module).join("|")})$`

That resolves to

^(_http_agent|_http_client|_http_common|_http_incoming|_http_outgoing|_http_server|_stream_duplex|_stream_passthrough|_stream_readable|_stream_transform|_stream_wrap|_stream_writable|_tls_common|_tls_wrap|assert|assert/strict|async_hooks|buffer|child_process|cluster|console|constants|crypto|dgram|diagnostics_channel|dns|dns/promises|domain|events|fs|fs/promises|http|http2|https|inspector|module|net|os|path|path/posix|path/win32|perf_hooks|process|punycode|querystring|readline|repl|stream|stream/consumers|stream/promises|stream/web|string_decoder|sys|timers|timers/promises|tls|trace_events|tty|url|util|util/types|v8|vm|wasi|worker_threads|zlib|node:_http_agent|node:_http_client|node:_http_common|node:_http_incoming|node:_http_outgoing|node:_http_server|node:_stream_duplex|node:_stream_passthrough|node:_stream_readable|node:_stream_transform|node:_stream_wrap|node:_stream_writable|node:_tls_common|node:_tls_wrap|node:assert|node:assert/strict|node:async_hooks|node:buffer|node:child_process|node:cluster|node:console|node:constants|node:crypto|node:dgram|node:diagnostics_channel|node:dns|node:dns/promises|node:domain|node:events|node:fs|node:fs/promises|node:http|node:http2|node:https|node:inspector|node:module|node:net|node:os|node:path|node:path/posix|node:path/win32|node:perf_hooks|node:process|node:punycode|node:querystring|node:readline|node:repl|node:stream|node:stream/consumers|node:stream/promises|node:stream/web|node:string_decoder|node:sys|node:timers|node:timers/promises|node:tls|node:trace_events|node:tty|node:url|node:util|node:util/types|node:v8|node:vm|node:wasi|node:worker_threads|node:zlib)$

IanVS added 2 commits May 17, 2022 15:30
@IanVS
Simplify failing test case
c9bc121
@IanVS
Make builtin regex more strict
495820a 
Previously, we could match on portions of relative paths.
This ensures that only an entire match of the builtin module names will be considered.
@IanVS
Copy link
Owner

IanVS commented May 17, 2022

Yep, exactly. Although I used a non-capturing group. I also simplified down the test case a bit.

blutorange
blutorange reviewed May 17, 2022
View reviewed changes
src/constants.ts Outdated
@@ -16,7 +16,7 @@ export const chunkTypeOther = 'other';
* where the not matched imports should be placed
*/
export const THIRD_PARTY_MODULES_SPECIAL_WORD = '<THIRD_PARTY_MODULES>';
export const BUILTIN_MODULES = builtinModules.join('|');
export const BUILTIN_MODULES = `^(?:${builtinModules.join('|')})$`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Careful, this is missing support for node protocol imports, e.g. require("node:fs")

`^(${builtinModules.join("|")}|${builtinModules.map(module => 'node:'+module).join("|")})$`

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps a bit simpler:

`^(node:)?(${builtinModules.join("|")})$`

Copy link
Owner

Choose a reason for hiding this comment

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

I should have run all the tests before pushing it up. :)

I tweaked the regex a bit to allow an optional node: prefix.

This comment was marked as outdated.

Sign in to view

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I see the fix now)

@IanVS
Copy link
Owner

IanVS commented May 17, 2022

Oh this is interesting. I guess that node:fs/promises wasn't valid in node 12. I'll adjust the tests a bit.

@IanVS
Copy link
Owner

IanVS commented May 17, 2022

I guess the other option is to stop supporting node 12. It's EOL, after all. I propose that we remove it from our tests, at least. What do you think, @blutorange, @fbartho?

@IanVS IanVS changed the title Test case for #16 More strict on what is considered a builtin May 17, 2022
@IanVS IanVS mentioned this pull request May 17, 2022
Merged
@fbartho
Copy link
Collaborator Author

fbartho commented May 17, 2022

I'm certainly supportive of only explicitly supporting N-1 on stable node release lines. -- Like maybe it works on node-12, but we shouldn't try to maintain support for it!

@fbartho fbartho requested review from blutorange and IanVS May 18, 2022 01:30
@fbartho
Copy link
Collaborator Author

fbartho commented May 18, 2022

The PR is green! Please let me know if you want me to strip out the test case, or move it somewhere, or if this PR is ready to go!

@fbartho fbartho changed the title More strict on what is considered a builtin More strict on what is considered a builtin - Fixes #16 May 18, 2022
IanVS
IanVS approved these changes May 18, 2022
View reviewed changes
Copy link
Owner

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks both of you.

@fbartho fbartho merged commit aa84b4d into IanVS:main May 18, 2022
@fbartho fbartho deleted the fb/repro-cli-issue branch May 18, 2022 01:48
@IanVS
Copy link
Owner

IanVS commented May 18, 2022

Released in https://github.com/IanVS/prettier-plugin-sort-imports/releases/tag/v3.4.2.

@IanVS IanVS mentioned this pull request May 18, 2022
Open
@fbartho fbartho mentioned this pull request May 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IanVS IanVS IanVS approved these changes

@blutorange blutorange Awaiting requested review from blutorange

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fbartho @IanVS @blutorange