ref: No more dynamic requires #801
Conversation
This patch changes all dynamic requires except for a few, 'safe' cases and 'sentry/cli' related ones as it will be removed in a follow up
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #801 +/- ##
==========================================
- Coverage 49.70% 49.67% -0.03%
==========================================
Files 52 52
Lines 3501 3503 +2
Branches 824 825 +1
==========================================
Hits 1740 1740
- Misses 1627 1628 +1
- Partials 134 135 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| try { | ||
| // If we run directly in setup-wizard | ||
| projectPackage = require('../../package.json'); | ||
| projectPackage = JSON.parse( | ||
| readFileSync('../../package.json', 'utf-8'), |
There was a problem hiding this comment.
Funny story about these: require is more fault tolerant than JSON.parse. Bun decided to permit comments in package.json files. Whatever drove them to make this decision is beyond me but here we are. JSON.parse will crash on Json with comments.
I opened #561 a while ago but closed it because it never came up in reality. I'm fine with using JSON.parse for the moment and revisit when people actually report crashes.
Gotta love the JS ecosystem
There was a problem hiding this comment.
It is tricky to make dynamic requires work with the static binaries. It is not impossible but is a bit more explicit (you need to use node:module and do makeRequire with an explicit starting path).
If we get complaints, I can switch to that.
This patch changes all dynamic requires except for a few, 'safe' cases and 'sentry/cli' related ones as it will be removed in a follow up