Skip to content

Duplicate "use strict" prologue in the generated output. #25550

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

Closed
mhegazy opened this issue Jul 10, 2018 · 10 comments
Closed

Duplicate "use strict" prologue in the generated output. #25550

mhegazy opened this issue Jul 10, 2018 · 10 comments
Assignees
Labels
Bug A bug in TypeScript Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Scenario: Monorepos & Cross-Project References Relates to composite projects (a.k.a references between "medium sized projects")

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Jul 10, 2018

Duplicate "use strict" prologue in the generated output.

See https://raw.githubusercontent.com/Microsoft/TypeScript/master/lib/tsc.js

@mhegazy mhegazy added the Bug A bug in TypeScript label Jul 10, 2018
@mhegazy mhegazy added this to the TypeScript 3.0.1 milestone Jul 10, 2018
@a-tarasyuk
Copy link
Contributor

a-tarasyuk commented Jul 12, 2018

@mhegazy @weswigham

TS transformer uses node.prepends, which includes build/local/compiler.js (createInputFiles, node.javascriptText). compiler.js has own "use strict" definition - it is the first "use strict"

The second "use strict" TS transformer adds in the method visitSourceFile because the following condition does not check "use strict" definition in the node.prepends

const alwaysStrict = getStrictOptionValue(compilerOptions, "alwaysStrict") &&
   !(isExternalModule(node) && moduleKind >= ModuleKind.ES2015) &&
   !isJsonSourceFile(node);

as the result, tsc.js contains "use strict";"use strict";

Does that make sense? What do you think?

@weswigham
Copy link
Member

Yep, that's exactly the problem. To fix it, we need to instantiate a scanner and scan the first of the prepends to see if it already has a use strict statement.

@weswigham
Copy link
Member

Actually, we should scan all of them and remove any prologues we find and unify them/move them to the top. That has interesting implications for souremapping, too.

@a-tarasyuk
Copy link
Contributor

a-tarasyuk commented Jul 13, 2018

@weswigham thanks.

Actually, we should scan all of them and remove any prologues we find and unify them/move them to the top. That has interesting implications for souremapping, too.

Should we remove prologues from prepends or check if prologues exist in some of them and skip adding "use strict" later? Why I ask, because "compiler.js" has "use strict", and in my opinion, it is right, because "compiler.js" independent file and it should contain "use strict"., just need to skip adding addition prologues later

@weswigham
Copy link
Member

weswigham commented Jul 17, 2018

@a-tarasyuk Keeping them in the intermediate outputs is fine (heck, we should probably even error if they don't all have the same set of prologues - concating a loose file to a strict one could be bad!) - we just need to remove them when concatenating and fixup the resulting concatenated sourcemaps.

@a-tarasyuk
Copy link
Contributor

a-tarasyuk commented Jul 17, 2018

@weswigham thanks. For instance, during concatenation, we need to remove "use strict" prologue directly in javascriptText(string representation). is it true? I just want to fix and make PR, if you don't mind. :)

@weswigham
Copy link
Member

I don't think we can remove it on load - since we need to track what we remove to update the sourcemap positions - it should probably just be elided during concatenation itself.

@a-tarasyuk
Copy link
Contributor

@weswigham I thought that we could track "use strict"; prologue when the compiler builds prepends - getPrependNodes, however, it is not true.

@weswigham
Copy link
Member

Yeah. Instead you wanna create a scanner and scan the text and check the type of the first token.

@DanielRosenwasser
Copy link
Member

@a-tarasyuk are you interested in sending a PR for this?

@DanielRosenwasser DanielRosenwasser added Help Wanted You can do this Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". labels Oct 30, 2018
@DanielRosenwasser DanielRosenwasser added the Scenario: Monorepos & Cross-Project References Relates to composite projects (a.k.a references between "medium sized projects") label Oct 30, 2018
@sheetalkamat sheetalkamat added Fixed A PR has been merged for this issue and removed Help Wanted You can do this labels Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Scenario: Monorepos & Cross-Project References Relates to composite projects (a.k.a references between "medium sized projects")
Projects
None yet
Development

No branches or pull requests

6 participants