-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(findExports): correctly dedup named exports #86
Conversation
// Prevent multiple exports of same function, only keep latest iteration of signatures | ||
const nextExport = exports[index + 1] |
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.
We were dedupe with the one that filtered out by AST. Changed to two passes of .filter
to ensure exports
is the real exports we want for dedupe.
Codecov Report
@@ Coverage Diff @@
## main nuxt/framework#86 +/- ##
==========================================
+ Coverage 67.48% 67.56% +0.07%
==========================================
Files 13 13
Lines 2196 2201 +5
Branches 233 233
==========================================
+ Hits 1482 1487 +5
Misses 695 695
Partials 19 19
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/analyze.ts
Outdated
return exportsLocation.some(location => exp.start <= location.start && exp.end >= location.end) | ||
return exportsLocation.some(location => | ||
(exp.start <= location.start && location.start <= exp.end) || | ||
(exp.start <= location.end && location.end <= exp.end) |
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.
Previous condition (token within match) seems keep tests pass. Why we need to change it to (token start within match || token end within match)?
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.
Made it into a comment to marge fix asap. fbf5c41. Would be happy to discuss more and change checking strategy if you think there is a case needs this check.
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.
That was my initial assumption of the cause - yes I think we don't need to change it for this case.
close nuxt/nuxt#14971