Fix require() to return mutable exports for Node.js compat#5847
Conversation
jasnell
left a comment
There was a problem hiding this comment.
This is likely breaking and I'm not yet convinced this is the correct fix. Specifically, we have the EXPORT_DEFAULT option and corresponding compat flag that is supposed to fix the issue. The problem is that the original implementation of require() returned the module namespace and not the default export. The compat flag is supposed to update that so that it returns the default export, which is the correct behavior. When that compat flag is not set, it's supposed to maintain the original behavior of returning the module namespace. Before applying any other fixes like this here, we need to investigate if/why the EXPORT_DEFAULT option is not being correctly set.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@jasnell I investigated and the EXPORT_DEFAULT mechanism is working correctly - the issue is much more in depth than it initially appeared.
I don't see any other way of solving this than the current approach. |
|
As mentioned, the proposed approach is likely breaking. This will require more consideration on what the correct approach is. I'm working on streams stuff at the moment but will be able to come back to this either later today or tomorrow. |
|
Ok, yeah I remember now about the limitations on the existing compat flag. What needs to happen then is introducing another compat flag that essentially says: for any module type, when require() is used, return the default export if it exists, and if it does not, fallback to the behavior you have here. However, if this new compat flag is turned off, the current behavior should be preserved. The general idea is that require should always return the default export, regardless of the module type. |
a74d0bf to
028bd7b
Compare
41a9458 to
f7c4cd0
Compare
vicb
left a comment
There was a problem hiding this comment.
When #5844 uses require, that's before the ESBuild bundling phase.
So there should be import in the end.
Note that the issue has import based code working in Node and failing in workerd.
So I think that's something we make sure works and should test.
I won't have time to check tonight but can probably find time to check by Monday.
Another option is to merge that but as experimental until we can double check it actually cover all the use cases and only remove exp / set a default date then.
vicb
left a comment
There was a problem hiding this comment.
I added a repro in #5844 (comment)
This PR does not fix it
7903a1b to
98fb96f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5847 +/- ##
==========================================
- Coverage 70.14% 70.14% -0.01%
==========================================
Files 407 407
Lines 107188 107225 +37
Branches 17965 17972 +7
==========================================
+ Hits 75189 75213 +24
- Misses 21216 21226 +10
- Partials 10783 10786 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
My concerns have been fixed in a different PR
Fixes #5844 (with the help of our friend Claude)
ES module namespaces have read-only properties by specification, but CommonJS require() is expected to return objects with mutable properties.