-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add strict mode directive to module bundles #8710
base: v2
Are you sure you want to change the base?
Conversation
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
5670c34
to
14014d6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
@@ -1 +1 @@ | |||
output = require('./'); | |||
global.output = require('./'); |
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.
Due to nodejs/node#38918 (comment) (only needed if the value being assigned is a function)
@@ -3611,7 +3611,8 @@ describe('scope hoisting', function () { | |||
assert.equal(output, 42); | |||
}); | |||
|
|||
it('builds commonjs modules that assigns to module.exports before exports', async function () { | |||
// TODO this should be working | |||
it.skip('builds commonjs modules that assigns to module.exports before exports', async function () { |
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.
The output right now is
parcelRequire.register("5CKoA", function(module, exports) {
module.exports = 42;
module.exports.foo = 27;
});
which is invalid in strict mode (something like "cannot add property to number")
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.
should we just change the first assignment to an object?
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.
Ah, forgot to include the input code:
module.exports = 42;
exports.foo = 27;
so this really is a failing test (just like the other one with the undeclared variable).
(The correct behaviour here is that "42" is exported and the second line does nothing)
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.
Seems fine. We should come back to the failing tests at some point.
@mischnic do you still want to merge this? |
Hi, thanks a lot for the progress so far! I was wondering if there is any plan to merge this fix, or if there is an alternative workaround/fix planned? Thanks again! |
Closes #8706