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.
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.
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
wontem
force-pushed
the
node_resolution_regex
branch
from
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! |
wontem
force-pushed
the
node_resolution_regex
branch
from
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.
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