Alias for commonjs require in JS #39770

sandersn · 2020-07-27T17:56:11Z

This PR binds commonjs const x = require('y') statements the same way as TS import x = require('y'): as an alias. This improves the consistency of JS, and should fix a number of bugs that have been particularly hard to fix over the years. It should let us get rid of quite a bit of ad-hoc code too. See below for details.

Fixes #38379 and
Fixes #37000
Fixes #25533

Maybe also

Generating Type Definitions from Javascript is blocked by requiring private name #37832 - edit: doesn't fix this, probably because of the fallback to require-as-call type checking.
Go to definition shows Node require statement as definition #39895 - fixes the import half of this, but exports also need to be aliases for goto-def to work exactly the same as ES.
JSDOC @type tag crashes with simple export #36830 - doesn't fix this -- it deals with the other branch of the ad-hoc code I want to get rid of
and some others I need to dig up, but I need to verify first.

Limitations and open questions:

There is special-case code in a few places for const x = require('y').z -- a require with a single suffix. But it doesn't work with const x = require('y').z.more.nested.accesses or const { destructuring } = require('y').z. This probably isn't much more, or much more complicated code -- I'm just not sure it's worth the effort, and I wanted to get an initial version done.
I didn't remove all of the existing special-case code. In particular, type checking of const s = require('fs').readFileSync("foo", "utf8") falls back to the old method of producing an anonymous type from require('fs') and then retrieving the property named readFileSync from that type.

The declaration emit change is easier to read with whitespace ignored because I indented a span of code without changing it.

taken from #39558 as soon as it was created

1. Simplify alias resolution. 2. Simplify variable-like checking. 3. Make binding skip require calls with type tags -- they fall back to the old require-call code and then check from there. I haven't started on the declaration emit code since I don't know what is going on there nearly as well.

Found one missing feature, not sure it's worth adding.

sandersn · 2020-07-27T17:57:36Z

src/compiler/binder.ts

@@ -3185,7 +3185,10 @@ namespace ts {
            }

            if (!isBindingPattern(node.name)) {
-                if (isBlockOrCatchScoped(node)) {
+                if (isInJSFile(node) && isRequireVariableDeclaration(node, /*requireStringLiteralLikeArgument*/ true) && !getJSDocTypeTag(node)) {


note that once this works well enough, we could turn it on in TS as well, which would be pretty great.

sandersn · 2020-07-27T17:58:00Z

src/compiler/checker.ts

-                                if (!associatedDeclarationForContainingInitializerOrBindingName) {
-                                    associatedDeclarationForContainingInitializerOrBindingName = location as BindingElement;
-                                }
+                            if (isParameterDeclaration(location as BindingElement) && !associatedDeclarationForContainingInitializerOrBindingName) {


driveby simplification to use isParameterDeclaration

src/compiler/checker.ts

sandersn · 2020-07-27T18:06:10Z

src/compiler/checker.ts

+                                // const x = require('y').z
+                                const initializer = (node as VariableDeclaration).initializer! as PropertyAccessExpression; // require('y').z
+                                const uniqueName = factory.createUniqueName((getExternalModuleRequireArgument(node) as StringLiteral).text); // _y
+                                const specifier = getSpecifierForModuleSymbol(target.parent || target, context); // 'y'


Since this is require('y').z, target is z and target.parent is require('y'). I fall back to target because, um, it MIGHT be right? And the declaration emitter seems to try to emit things even if they're not correct? Maybe this whole block should instead check that target.parent is defined before proceeding. @weswigham what do you think?

sandersn · 2020-07-27T18:06:56Z

src/compiler/checker.ts

-                        }
-                        isRequireAlias = isCallExpression(expr) && isRequireCall(expr, /*requireStringLiteralLikeArgument*/ true) && !!valueType.symbol;
-                    }
+                    const isRequireAlias = isRequireVariableDeclaration(symbol.valueDeclaration, /*requireStringLiteralLikeArgument*/ true);


another driveby simplification, this time to use isRequireVariableDeclaration.

Also, isRequireAlias should never be true now, so I think this can be removed entirely. But I need to test it.

elibarzilay

It looks fine to me, but in the meat of the changes I probably followed around 25% of it so it's not worth much...

src/compiler/utilities.ts

src/compiler/checker.ts

sandersn · 2020-08-11T16:02:44Z

The latest merge from master shows that the nested-import generation in #39818 still does not work for require. I'll need to go find out why.

Ad-hoc type reference resolution for `require` isn't needed anymore.

sandersn · 2020-08-13T16:57:17Z

src/compiler/checker.ts

+                && isAliasableOrJsExpression(node.parent.right)
+                || node.kind === SyntaxKind.ShorthandPropertyAssignment
+                || node.kind === SyntaxKind.PropertyAssignment && isAliasableOrJsExpression((node as PropertyAssignment).initializer)
+                || isRequireVariableDeclaration(node, /*requireStringLiteralLikeArgument*/ true);


this is the new line of code, the rest is reformatting requested in the review

sandersn · 2020-08-13T16:59:29Z

src/services/goToDefinition.ts

-        if (symbol && isInJSFile(node)) {
-            const requireCall = forEach(symbol.declarations, d => isVariableDeclaration(d) && !!d.initializer && isRequireCall(d.initializer, /*checkArgumentIsStringLiteralLike*/ true) ? d.initializer : undefined);
-            if (requireCall) {
-                const moduleSymbol = checker.getSymbolAtLocation(requireCall.arguments[0]);


this ad-hoc code has been replaced by the call to checker.getAliasedSymbol. I just had to add require calls to the list of aliases to skip past.

sandersn · 2020-08-13T17:08:49Z

tests/cases/fourslash/goToDefinitionJsModuleNameAtImportName.ts

@@ -3,10 +3,11 @@
 // @allowJs: true

 // @Filename: /foo.js
-//// /*moduleDef*/class Blah {
+//// /*moduleDef*/function notExported() { }


this gotoDef test

appears to show that goto-def on the imported module symbol jumps to the single export of the module,
but actually shows that goto-def on the imported module symbol jumps to the very beginning of the module.

I added a non-exported function to make this clearer.

sandersn · 2020-08-13T17:09:08Z

@weswigham this should be ready to review now.

weswigham

Looks like this ended up being way simpler than it looked. So, (at least one of) the goal(s) with doing these aliases properly is removing the value-side fallback lookup from the isJSDocTypeReference case in getTypeFromTypeReference - can we do that?

src/compiler/checker.ts

weswigham · 2020-08-13T21:15:14Z

src/compiler/checker.ts

+                                const uniqueName = factory.createUniqueName((getExternalModuleRequireArgument(node) as StringLiteral).text); // _y
+                                const specifier = getSpecifierForModuleSymbol(target.parent || target, context); // 'y'
+                                // import _y = require('y');
+                                addResult(factory.createImportEqualsDeclaration(


So, this always needs to get made in the outermost scope, rather than the current (potentially nested) one, since import x = require('whatever') isn't valid syntax within a namespace. includePrivateSymbol handles that for specific symbols now (by adding the symbol we want to serialize to the private symbol list for the outermost scope), however we don't have a good mechanism (currently) for adding parts of serialized results to differing scopes. You might just need a sidechannel in the NodeBuilderContext that you add this declaration do that you then grab and append to the start/end (probably doesn't matter which) of the file once you're doing serializing all symbols.

As for a test case for this... something like a module.exports.A.B.C export that relies on a require'd type alias?

src/compiler/checker.ts

sandersn · 2020-08-13T21:47:45Z

removing the value-side fallback lookup from the isJSDocTypeReference case in getTypeFromTypeReference - can we do that?

No, for two reasons. First, this doesn't make all exports into aliases.
Second, the value side fallback is inside of a module also used for things like

var exports = {}
/** @typedef {string} */
exports.SomeName;
/** @type {exports.SomeName} */
const myString = 'str';

const o = { a: 1, b: 2, c: 3 }
/** @type {keyof o} */
var x

var Outer = function O() { this.y = 2 }
Outer.Inner = class I {
  constructor() {
    this.x = 1
  }
}
/** @type {Outer} */var z
/** @type {Outer.Inner} */var ka

It does fix about 20% of the 50-odd failures we used to get without this fallback, but that's probably because we're importing classes correctly now.

1. Fix indentation 2. Add comment for exported JSON emit 3. Add test case for nested-namespace exports.

sandersn added 24 commits July 8, 2020 17:00

First attempt at aliases for require

4f9ccd0

Merge branch 'master' into alias-for-require

d692387

test+initial support for const x=require

277d52a

1st round of baseline improvements

bcdc2e4

2nd round of baseline updates

20aa6ed

support property access after require

f213707

check @type tag on require

122babd

Merge branch 'master' into alias-for-require

f32ec4e

forbid expando missing namespaces on aliases

e1e32af

taken from #39558 as soon as it was created

accept error baselines that are good, actually

4ddb3d0

Scribbling on d.ts emit code

58a0b65

use getSpecifierForModuleSymbol

d885b67

hideous hack for module.exports of aliases

5a0b092

Merge branch 'master' into alias-for-require

64c8c86

Fix module.exports.x --> export list emit

55a13b7

fix isLocalImport predicate

7ce7c8c

require only creates aliases in JS

ca5a57b

re-handle json imports

e9a355d

update fourslash baseline

febcf0e

Function for getting module name from require call

a671b45

Merge branch 'master' into alias-for-require

6dfbec3

First round of cleanup plus a new test

0c92277

Found one missing feature, not sure it's worth adding.

more small cleanup

dee0561

typescript-bot assigned sandersn Jul 27, 2020

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 27, 2020

sandersn commented Jul 27, 2020

View reviewed changes

sandersn requested review from elibarzilay and weswigham July 27, 2020 18:08

Merge branch 'master' into alias-for-require

a841988

elibarzilay approved these changes Jul 30, 2020

View reviewed changes

src/compiler/utilities.ts Outdated Show resolved Hide resolved

src/compiler/checker.ts Outdated Show resolved Hide resolved

elibarzilay mentioned this pull request Aug 4, 2020

jsdoc @enum with import/export #33575

Closed

Merge branch 'master' into alias-for-require

0694890

sandersn mentioned this pull request Aug 11, 2020

Go to definition shows Node require statement as definition #39895

Closed

sandersn added 7 commits August 11, 2020 10:19

Code review comments, plus remove unneeded code

781bec2

Ad-hoc type reference resolution for `require` isn't needed anymore.

find all refs works

2bfeeeb

Merge branch 'master' into alias-for-require

0ba0d90

remove old ad-hoc code

62e5b18

make it clear that old behaviour is not that correct

2679b03

update api baselines

03fb2d4

remove outdated comment

7aeb306

sandersn commented Aug 13, 2020

View reviewed changes

weswigham approved these changes Aug 13, 2020

View reviewed changes

Merge branch 'master' into alias-for-require

d2c0b3c

sandersn added 3 commits August 17, 2020 10:54

Merge branch 'master' into alias-for-require

9d2ce87

PR feedback

283a520

1. Fix indentation 2. Add comment for exported JSON emit 3. Add test case for nested-namespace exports.

add a fail-case test (which passes!)

6c7897e

sandersn merged commit c3d41bb into master Aug 17, 2020

sandersn deleted the alias-for-require branch August 17, 2020 21:00

This was referenced Aug 18, 2020

JSDOC @type tag crashes with simple export #36830

Closed

Stop an infinite loop when using an exported JSDoc function as a type #38332

Closed

JoostK mentioned this pull request Nov 6, 2020

Support TypeScript 4.1 angular/angular#39571

Closed

a-tarasyuk mentioned this pull request Nov 24, 2020

TS >=4.1: Cannot assign because it is not a variable (require does not work with let anymore) #41628

Closed

This was referenced Nov 25, 2020

TS 4.1.2 Cannot find name 'FieldValue' (solved in 4.2) #41629

Closed

[JS] accessing nested namespaces from require() with destructuring leads to an error #41699

Open

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alias for commonjs require in JS #39770

Alias for commonjs require in JS #39770

sandersn commented Jul 27, 2020 •

edited

Loading

sandersn Jul 27, 2020

sandersn Jul 27, 2020

sandersn Jul 27, 2020 •

edited

Loading

sandersn Jul 27, 2020

elibarzilay left a comment

sandersn commented Aug 11, 2020

sandersn Aug 13, 2020

sandersn Aug 13, 2020

sandersn Aug 13, 2020

sandersn commented Aug 13, 2020

weswigham left a comment

weswigham Aug 13, 2020

weswigham Aug 13, 2020

sandersn commented Aug 13, 2020

Alias for commonjs require in JS #39770

Alias for commonjs require in JS #39770

Conversation

sandersn commented Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elibarzilay left a comment

Choose a reason for hiding this comment

sandersn commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Aug 13, 2020

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Aug 13, 2020

sandersn commented Jul 27, 2020 •

edited

Loading

sandersn Jul 27, 2020 •

edited

Loading