Skip to content
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

const/let variables with require() initializer are not correctly block-scoped #41697

Closed
sokra opened this issue Nov 26, 2020 · 4 comments
Closed
Assignees
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@sokra
Copy link
Contributor

sokra commented Nov 26, 2020

TypeScript Version: 4.2.0-dev.20201126

Search Terms: TS2300

Code

// compiled in JS mode. Works fine when compiled as TS
switch(something) {
    case "a": {
        const x = require("fs"); // TS2300: Duplicate identifier 'x'
        console.log(x);
        break;
    }
    case "b": {
        const x = require("fs"); // TS2300: Duplicate identifier 'x'
        console.log(x);
        break;
    }
    case "c": {
        const x = 123; // No error (only happens with require() as initializer)
        console.log(x);
        break;
    }
}
x.readFileSync("x"); // should be error

Expected behavior:

No TS2300 error. const variables should be block-scoped.

Actual behavior:

const variables with require() initializer leak into outer scope. Looks like they are incorrectly function scoped.

Playground Link: Playground link

Related Issues:

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Nov 26, 2020

That’s because in JS code, TypeScript treats

const x = require("foo");

as

import x = require("foo");

which gets transpiled to

var x = require("foo");

There’s no easy fix other than supporting #31090 and defining require as:

declare function require<T extends string>(module: T): typeof import(T);

@sk-
Copy link

sk- commented Nov 27, 2020

Thu culprit seems to be #39770. Unfortunately there was no mention of this change in the release notes and there are many related issues.

sokra added a commit to webpack/webpack that referenced this issue Nov 28, 2020
sokra added a commit to webpack/webpack that referenced this issue Nov 28, 2020
sokra added a commit to webpack/webpack that referenced this issue Nov 30, 2020
@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Dec 2, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.2.0 milestone Dec 2, 2020
@RyanCavanaugh
Copy link
Member

Unfortunately there was no mention of this change in the release notes

We didn't realize it was a break at the time -- our bad!

@sandersn
Copy link
Member

sandersn commented Feb 3, 2021

I surveyed Definitely Typed, our user tests and all the code in their node_modules to see how often this is used, and found that 5 projects used it outside of generated code or tests.

The total number of duplicated imports is quite small: 55 out of 71,000 non-toplevel require calls in aws-sdk, @babel/core, webassembly-feature and read-installed:

  • require json files in different functions, all with the same type
  • require different protocol handlers, all with the same type
  • some kind of delayed evaluation pattern
  • codegen repeated require calls for feature detection
  • conditionally require('graceful-fs') with a fallback to fs. I assume they must be treated as if they have the same type.

So:

  • this affects a small proportion of non-toplevel imports
  • most JS users don't have checkJS on, which would not report the error, but give the error semantics where the first require's type is used.
  • more than half of the duplicated requires gave [nearly] the same type to every require, which would make the error semantics [nearly] the same as correct semantics
  • block-scoping wouldn't give the correct semantics to the codegen examples, just silence the error.

I think the value of just silencing the error is pretty low since it only helps checkJS code bases, and we expect checkJS code bases to be on the way to using TS/ES imports anyway. Especially if the cost is making the 71,000 other non-toplevel require back into non-aliases.

@sandersn sandersn closed this as completed Feb 3, 2021
@sandersn sandersn added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

6 participants