fix: swallow more require errors from *ts files#5498
Conversation
|
See run 4329 for test results |
|
No new failures, ready for review. Though Coveralls was down so all test jobs failed 😭 I checked the integration ones and nothing went wrong there! |
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Thank you for so thoroughly covering this silly oversight of mine! +1 to the new form of test coverage.
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
PostmortemI never fully wrapped my head around the exact problem here. Why was Node loading modules in a different way based on whether I'll be updating this comment as I reflect more, I plan to reproduce the issue in excruciating detail so that I can summarize it precisely. Open questions
References
|
|
@copilot draft a report about why this error happened, it's quite complex. Organize sources so that the report is readable. Try to answer the open questions in this comment thread. |
PR Checklist
status: accepting prsOverview
In #5494, users reported
requireErrbeing thrown whenimportErrshould've been thrown instead. Recent change #5408 accidentally throwsrequireErrwhen the file is a *ts file orimportErr.code === 'ERR_UNKNOWN_FILE_EXTENSION'. This condition should've been and, because we don't want to throw the earlierrequireErrwhen the laterimportErris meaningful (e.g. from a *ts file but not an unknown file extension).Per research into #5361 (comment), we know Node's loader / "customization hook" behavior changes whether the parent package is in module mode or not. Because our CI tests run through
nyc, Node was always "primed" to import modules, soimportErrandrequireErrwere the same in tests that expected them to be different. This is why we didn't detect the issue earlier.This PR adds
test-node-integration:bareto the only test that needs it. Bare tests are run directly vianpx mochato avoid accidentally "priming" Node to import modules.(This is a PR into the new
v11.xbranch, so we'll have to find a way to push 11.7.5 instead of 12.0.0-beta-1. Not sure if we want this to be a one-off for now or work towards a high-resilience deployment system. We haven't defined our support of old Mocha versions, but I believe we should be generous in backporting given our userbase)