-
Notifications
You must be signed in to change notification settings - Fork 563
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
Update packages to be browser-first and use separate export for Node.js #2211
Changes from all commits
b5426b8
253291a
6ba5a4a
8dcb263
7f93875
a1924ee
5a7c7cf
b9fc03a
8f9d87a
6b573ef
7b04230
1edf6f7
0b48769
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
diff --git a/lib/try-path.js b/lib/try-path.js | ||
index de11ccbf9090658eb0ee61bdba1984ee21bcb3b7..2e75f72483aa7b05bf0a2cde06b3c009eb2103e8 100644 | ||
--- a/lib/try-path.js | ||
+++ b/lib/try-path.js | ||
@@ -85,6 +85,6 @@ function matchStar(pattern, search) { | ||
if (search.substr(search.length - part2.length) !== part2) { | ||
return undefined; | ||
} | ||
- return search.substr(star, search.length - part2.length); | ||
+ return search.substr(star, search.length - part2.length - part1.length); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to try and upstream this. Also we need to determine if it is a bug before merging 😓 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a ~3.5 year old issue and PR open in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we try opening a well-reasoned PR with tests etc that is an easy merge for the maintainers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but do you think that should block merging this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to get confirmation that it's a bug, but probably not blocking |
||
} | ||
//# sourceMappingURL=try-path.js.map | ||
\ No newline at end of file |
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'm not sure if this is a bug or not, but the previous behaviour was unexpected (and I'm not sure how this worked before). Basically when checking a
tsconfig.json
path with a wildcard, this function returns the replacement for the path.@metamask/*/node
Input:
@metamask/snaps-utils/node
Result:
snaps-utils
@metamask/*
Input:
@metamask/snaps-controllers
Result:
snaps-controllers
And so on. Before this change however, it would return:
@metamask/*/node
Input:
@metamask/snaps-utils/node
Result:
snaps-utils/node
Meaning that it would resolve to
packages/snaps-utils/node/src/...
and some other invalid paths. This causes it to fall back to Node.js' default resolver, which would look at thepackage.json
ofsnaps-utils
and try to loadpackages/snaps-utils/dist/node.js
, but we can't do that since the file may not be built yet.