Fix node 20 reexports #30
Conversation
|
@luxaritas I see this is a draft... Is it ready to review? |
|
Hey @tlhunter - main reason I left it as a draft is because I'm not confident that my approach here is "correct" - particularly how I'm handling path resolution seems hacky. That said I don't have any pending work on it, really just waiting for input, so I'll mark it as ready for review |
There was a problem hiding this comment.
Thank you for implementing this!
I found a problem with circular imports that re-export. The current implementation will go into an infinite loop. This patch includes a test case for that and a proposed fix:
https://gist.github.com/trentm/aa10ec6c34a473aa64a2e717b931742c
@luxaritas Would you be willing to add that to your branch?
lib/get-exports.js
Outdated
| return Array.from(new Set([ | ||
| ...addDefault(ex.exports), | ||
| ...(await Promise.all(ex.reexports.map(re => getExports( | ||
| re.startsWith('./') || re.startsWith('../') |
There was a problem hiding this comment.
Possibly this should consider Windows \\ dir separators, if on Windows? I'm not sure how important that is, and it could be done later.
There was a problem hiding this comment.
To be honest the entire path handling here felt hacky when I wrote it, and I'm not sure if this is the best way to do it, though I didn't immediately come up with a better option. :)
As far as windows dir separators - I suppose? I think I initially thought cjs-module-lexer would normalize this, but in retrospect that's probably not correct. This also made me realize that importing . or .. would currently not be handled correctly either - I'll fix that up
|
@trentm Thanks for the patch - looks reasonable to me so I've applied it. (I've marked you as a commit coauthor using git metadata from a recent GitHub commit of yours, let me know if that should be something else!) |
|
If I'm not mistaken, this is solved by #43. |
Co-authored-by: Trent Mick <trentm@gmail.com>
…ut trailing slash
rochdev
force-pushed
the
fix/node-20-reexports
branch
from
e045434 to
d57d1b1
Compare
|
Woot. Thanks very much. |
- Fixes node 20 reexports nodejs/import-in-the-middle#30 - Fixes duplicate default exports nodejs/import-in-the-middle#53
) - Fixes node 20 reexports nodejs/import-in-the-middle#30 - Fixes duplicate default exports nodejs/import-in-the-middle#53
Resolves #29