Skip to content

Emit modules in strict mode #5765

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

Merged
merged 10 commits into from
Nov 24, 2015

Conversation

weswigham
Copy link
Member

Fixes #3576.
Closes #4963.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 24, 2015

it is common for users to have "use strict" as their first line in a module anyways, now we will emit two.

@weswigham
Copy link
Member Author

Correct! But this change would bring us inline with both babel and our own parser (since we parse in strict mode in modules). There's also no harm in it. (And users can easily remove the extra "use strict" form their own code if it bothers them)

@weswigham
Copy link
Member Author

An interesting behavioral quirk this fixes is this:

export function what() {
  return this;
}
var x = what.call(2);
assert(typeof x === "number");

That assertion changes truthiness based on the strict mode status of the containing code. (Specifically, its only true in strict mode.)
So compiling this code in babel, the assertion is true, but with TS it is false at present.

@DanielRosenwasser
Copy link
Member

You could just check if the first statement is a "use strict" directive and omit it if it is.

@weswigham
Copy link
Member Author

@DanielRosenwasser So you have asked, so you shall recieve

@yuit
Copy link
Contributor

yuit commented Nov 24, 2015

Look good 👍 Could you add a test when users already have "use strict" and we omit the emitting.

@DanielRosenwasser
Copy link
Member

💯

@weswigham
Copy link
Member Author

@yuit tests/baselines/reference/modulePrologueAMD.js, tests/baselines/reference/modulePrologueCommonjs.js, tests/baselines/reference/modulePrologueSystem.js, and tests/baselines/reference/modulePrologueUmd.js got ya covered

weswigham added a commit that referenced this pull request Nov 24, 2015
@weswigham weswigham merged commit d0de238 into microsoft:master Nov 24, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Nov 25, 2015

@weswigham can you add a blurb about this change and what breaks in the breaking changes page; mainly issues that the compiler will not catch but will break at runtime, e.g. eval introducing new variables, value of this, errors on calling function.caller, etc.. possibly reference https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants