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

Resolve the modue names with "modulename.json" to json files when doing node module resolution #22167

Merged
merged 25 commits into from May 4, 2018

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Feb 24, 2018

Fixes #7071

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented Feb 26, 2018

@mhegazy c154b81 is the issue that was causing emptyArray to modify and my tests to fail.

@calebboyd
Copy link

@calebboyd calebboyd commented Mar 8, 2018

This is great! Any chance it will make it into 2.8?

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented Mar 10, 2018

@mhegazy can you please take a look so we can get this in. Thank you.

Copy link
Member

@sandersn sandersn left a comment

The binder and checker code look good. I'll have to look at the rest later.

One big question: why doesn't the flag default to true, at least for Javascript files? In fact, why is there a flag at all?

If there is a difference between JS and TS, then there should be a JS-specific test.

Copy link
Member

@sandersn sandersn left a comment

Some comments and questions, plus I think this should default to true for JS. (Actually, it could probably default to true for TS if our main concern about this feature is about taking too long to compile very large json files.)


function convertObjectLiteralExpressionToJson(
node: ObjectLiteralExpression,
knownOptions: Map<CommandLineOption> | undefined,
extraKeyDiagnosticMessage: DiagnosticMessage | undefined,
parentOption: string | undefined
): any {
const result: any = {};
const result: any = returnValue ? {} : undefined;
Copy link
Member

@sandersn sandersn Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this returnValue check just for efficiency, to avoid building up a value that's not needed.

Actually, why isn't it needed, when existing callers needed it?

Copy link
Member Author

@sheetalkamat sheetalkamat Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont need value when we are importing modules: We just want the error reporting on valid json syntax. So creating (potentially large) json object isnt needed hence the flag. In case of config file, we do need json object as well as checks which can be done simultaneously.

Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment on the function header?

// Set source file so that errors will be reported with this file name
sourceFile = createSourceFile(fileName, ScriptTarget.ES2015, ScriptKind.JSON, /*isDeclaration*/ false);
const result = <JsonSourceFile>sourceFile;
const result = sourceFile as JsonSourceFile;
Copy link
Member

@sandersn sandersn Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code modifies sourceFile directly. I don't think result is really used anymore, so I'd just return sourceFile at the end of this function and get rid of result..

else {
const statement = createNode(SyntaxKind.ExpressionStatement) as JsonObjectExpressionStatement;
switch (token()) {
case SyntaxKind.OpenBracketToken:
Copy link
Member

@sandersn sandersn Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it that we didn't parse top-level arrays, etc, in json files until this change?

Copy link
Member

@sandersn sandersn Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be easier to parseExpression and then have an error for expression kinds that are not legal? I don't think we're checking the contents of, say, parseObjectLiteralExpression either, so there's not actually much point in being strict here.

Copy link
Member Author

@sheetalkamat sheetalkamat Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue in that is we dont do better job with incorrect object literal expression .Eg. There is already test (https://github.com/Microsoft/TypeScript/blob/master/src/harness/unittests/projectErrors.ts#L116) for "files": something} where in the first bracket is missing. We cant default to object literal which would be very likely the contents in json

Copy link
Member

@sandersn sandersn Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok. I didn't realise that this approach gives better error recovery.

@@ -1983,7 +1983,7 @@ namespace ts {
}
Copy link
Member

@sandersn sandersn Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting to see permissions changing from 755 to 644. Any idea why they were 755 before?

Copy link
Member Author

@sheetalkamat sheetalkamat Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what?

Copy link
Member

@sandersn sandersn Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the title for program.ts, it says that program.ts has 14 lines changed and also changed permissions from 100755 to 100644. I think that is safer since I don't think we can run program.ts from the command line, but I wasn't sure what prompted the change.

@@ -2608,10 +2609,23 @@ namespace ts {
}

export interface JsonSourceFile extends SourceFile {
jsonObject?: ObjectLiteralExpression;
statements: NodeArray<JsonObjectExpressionStatement>;
Copy link
Member

@sandersn sandersn Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the reason for this -- ObjectLiteralExpression can contain methods, so JsonObjectExpressionStatement is not actually that restrictive. Why not just use ExpressionStatement here?

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented Apr 30, 2018

One big question: why doesn't the flag default to true, at least for Javascript files? In fact, why is there a flag at all?

Because we dont want to pull in big json files. Its almost too late most of the time when the json could result into big memory. It could be too many options or too long a string. it was discussed in design note to put this under the flag instead.
Having people to consciously opt into this would imply the user understands the cost.

Copy link
Member

@sandersn sandersn left a comment

I think it's ready to go, although I'd still like to know whether JsonSourceFile.statements could have type NodeArray<ExpressionStatement>.

After our in-person discussion, I think it's ok to start with a flag required for both JS and TS files, and perhaps revisit this if there's enough demand for better types in config-free scenarios.

else {
const statement = createNode(SyntaxKind.ExpressionStatement) as JsonObjectExpressionStatement;
switch (token()) {
case SyntaxKind.OpenBracketToken:
Copy link
Member

@sandersn sandersn Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok. I didn't realise that this approach gives better error recovery.

@@ -1983,7 +1983,7 @@ namespace ts {
}
Copy link
Member

@sandersn sandersn Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the title for program.ts, it says that program.ts has 14 lines changed and also changed permissions from 100755 to 100644. I think that is safer since I don't think we can run program.ts from the command line, but I wasn't sure what prompted the change.

@@ -1671,7 +1675,9 @@ namespace ts {

function emitExpressionStatement(node: ExpressionStatement) {
emitExpression(node.expression);
writeSemicolon();
if (!isJsonSourceFile(currentSourceFile)) {
Copy link
Contributor

@mhegazy mhegazy Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would this be called in the first place?

Copy link
Member Author

@sheetalkamat sheetalkamat May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Microsoft/TypeScript/pull/22167/files#diff-9e36f50e46928676c6142f634cb7cd1bR18 when the output is copied into outDir just like any other js file ?

Copy link
Contributor

@mhegazy mhegazy May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see

@@ -4685,6 +4685,11 @@ namespace ts {
return links.type = anyType;
}
// Handle export default expressions
if (declaration.kind === SyntaxKind.SourceFile) {
Copy link
Contributor

@mhegazy mhegazy Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isSourceFile(declaration) and save the cast few lines in

@@ -505,6 +505,7 @@ namespace ts {
JSDoc = 1 << 21, // If node was parsed inside jsdoc
/* @internal */ Ambient = 1 << 22, // If node was inside an ambient context -- a declaration file, or inside something with the `declare` modifier.
/* @internal */ InWithStatement = 1 << 23, // If any ancestor of node was the `statement` of a WithStatement (not the `expression`)
JsonFile = 1 << 24, // If node was parsed in a Json
Copy link
Contributor

@mhegazy mhegazy Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would just call it JSON

Copy link
Contributor

@mhegazy mhegazy Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or Json for consistency

Copy link
Member Author

@sheetalkamat sheetalkamat Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Microsoft/TypeScript/pull/22167/files#diff-4b8bd1eea29904f1be39cd864e1a45c0R489 has this as JavaScriptFile isnt it more consitent to use JsonFile ?

Copy link
Contributor

@mhegazy mhegazy May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure..

@mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Apr 30, 2018

@andy-ms can you please review this change.

@mhegazy mhegazy requested a review from andy-ms Apr 30, 2018
@mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Apr 30, 2018

@sheetalkamat, a couple of comments:

  1. I think we need a size limit as well. either in the module resolver or in the program. in .js files these can be big, and we do not want to kill the server if one (or few) were loaded.
  2. we should enable it for js projects by default, in other words set --resolveJsonModules to true in getDefaultCompilerOptions in a jsconfig.json. and also in the server. we can do that in a separate PR if you want.

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented May 3, 2018

chatted with @mhegazy offline and the making --resolveJsonModules default to true for javascript files and limiting the size of files need to be handled separately since its going to involve effort thats generic even for javascript projects in general.

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented May 3, 2018

Ping @andy-ms @mhegazy for final review before merge

@@ -4718,6 +4718,11 @@ namespace ts {
return links.type = anyType;
}
// Handle export default expressions
if (isSourceFile(declaration)) {
Debug.assert(isJsonSourceFile(declaration));
const jsonSourceFile = <JsonSourceFile>declaration;
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cast(declaration, isJsonSourceFile) helps avoid the type assertion

@@ -3543,6 +3547,11 @@
"code": 6196,
"reportsUnnecessary": true
},
"Resolve module name imported with '.json' extension to the json source file.": {
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this message could be improved -- not obvious what the alternative is. "Type-check imported '.json' files" might be a better explanation. The same for the error message -- it's not the module resolution that users will care about, but the fact that we typecheck the contents of the file.

Copy link
Member Author

@sheetalkamat sheetalkamat May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's not what it means though.. We wont even look for .json files if the flag is on. So its not just typechecking but picking up the modules?

Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "Include '.json' files in the program"?
The reason is that module resolution seems like more of an internal issue that most users won't care about. But they will care if their '.json' files are getting compiled or not.

Copy link
Member Author

@sheetalkamat sheetalkamat May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somehow module needs to be part of the msg since aren't just including any json file?
Include modules imported with '.json' extension ?

Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that .json imports would have failed before, that's a better description.


function convertObjectLiteralExpressionToJson(
node: ObjectLiteralExpression,
knownOptions: Map<CommandLineOption> | undefined,
extraKeyDiagnosticMessage: DiagnosticMessage | undefined,
parentOption: string | undefined
): any {
const result: any = {};
const result: any = returnValue ? {} : undefined;
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment on the function header?

@@ -898,6 +903,11 @@ namespace ts {
* in cases when we know upfront that all load attempts will fail (because containing folder does not exists) however we still need to record all failed lookup locations.
*/
function loadModuleFromFile(extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState): PathAndExtension | undefined {
if (extensions === Extensions.Json) {
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this could be handled along with the bottom case if (hasJavaScriptFileExtension)?

Copy link
Member Author

@sheetalkamat sheetalkamat May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That just makes too many checks of extension !== Extensions.Json since we dont want to resolve "module" to module.json but only "module.json"

@@ -2426,6 +2433,7 @@ namespace ts {
switch (extension) {
case Extension.Ts:
case Extension.Dts:
case Extension.Json:
// These are always allowed.
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we would only ever get a .json module if the option was set? Maybe could put that in the comment.

}
}

export function getTsConfigPropArrayElementValue(tsConfigSourceFile: TsConfigSourceFile, propKey: string, elementValue: string): StringLiteral {
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: StringLiteral | undefined


export function getTsConfigPropArrayElementValue(tsConfigSourceFile: TsConfigSourceFile, propKey: string, elementValue: string): StringLiteral {
const jsonObjectLiteral = getTsConfigObjectLiteralExpression(tsConfigSourceFile);
if (jsonObjectLiteral) {
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return jsonObjectLiteral && firstDefined(getPropertyAssignment(jsonObjectLiteral, propKey), property =>
    isArrayLiteralExpression(property.initializer)
        ? find(property.initializer.elements, (element): element is StringLiteral => isStringLiteral(element) && element.text === elementValue)
        : undefined);

@@ -1217,7 +1217,7 @@ namespace Harness {
}

const useCaseSensitiveFileNames = options.useCaseSensitiveFileNames !== undefined ? options.useCaseSensitiveFileNames : true;
const programFileNames = inputFiles.map(file => file.unitName);
const programFileNames = inputFiles.map(file => file.unitName).filter(fileName => !ts.fileExtensionIs(fileName, ts.Extension.Json));
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why filter these?

Copy link
Member Author

@sheetalkamat sheetalkamat May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because json files arent provided as fileNames but only picked during module resolution

if (!cfg) return;
const oldFile = cfg.jsonObject && getFilesEntry(cfg.jsonObject, oldFilePath);
const configFile = program.getCompilerOptions().configFile;
const oldFile = getTsConfigPropArrayElementValue(configFile, "files", oldFilePath);
Copy link
Contributor

@andy-ms andy-ms May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: That function isn't declared to accept undefined inputs

andy-ms
andy-ms approved these changes May 4, 2018
@@ -2200,6 +2200,12 @@ namespace ts {
}
}

if (options.resolveJsonModule) {
if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.NodeJs) {
Copy link
Contributor

@mhegazy mhegazy May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about --module? do not think we should allow this for --module ES2015 for example..

Copy link
Member Author

@sheetalkamat sheetalkamat May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified offline. At present --moduleResolution specified takes preferences for resolution irrespective of --module and since there is no change in there, no error needs to be reported specially.

mhegazy
mhegazy approved these changes May 4, 2018
Copy link
Contributor

@mhegazy mhegazy left a comment

One comment about --module and --resolveJsonModule relationship.

@mhegazy mhegazy mentioned this pull request May 4, 2018
4 tasks
@sheetalkamat sheetalkamat merged commit 5133c70 into master May 4, 2018
5 checks passed
@sheetalkamat sheetalkamat deleted the requireJson branch May 4, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
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.

None yet

5 participants