-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
fix(npm): use original node regex in npm resolution #17404
Conversation
ext/node/resolution.rs
Outdated
@@ -339,7 +339,7 @@ fn resolve_package_target_string( | |||
)); | |||
} | |||
let invalid_segment_re = | |||
Regex::new(r"(^|\|/)(..?|node_modules)(\|/|$)").expect("bad regex"); | |||
Regex::new(r"(^|\\|/)(\.\.?|node_modules)?(\\|/|$)").expect("bad regex"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The corresponding regex in the node codebase:
After cleanup I found that not only escaping symbols are missed but also the question mark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the optional regex group change because I found that it was introduced in this PR (related to double slash deprecation), after the deno impelementation so I guess, it's enough to have the deprecated version for now
a193f5f
to
b46a8c7
Compare
Cool, thanks for the fix! Any chance you could add some tests that exercise this changed regex? I believe they should be added to |
Sure! Just wanted to ask about tests. Thanks for the quick comment! |
7920aad
to
7149c7f
Compare
Added a test case and reverted the optional regex group change, see #17404 (comment) |
The failed test seems to be unrelated. Can it be flaky? |
That's possible, I restarted the CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I confirmed the test fails on main
but this PR fixes it. Very nice first contribution @wontem, thanks!
Fixes regex for matching conditional exports in a package. Updated to the same regex Node.js uses.
Closes #17403