GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Resolve the modue names with "modulename.json" to json files when doing node module resolution by sheetalkamat · Pull Request #22167 · microsoft/TypeScript

Conversation

@sheetalkamat

@sheetalkamat Sheetal Nandi (sheetalkamat) commented Feb 24, 2018

Copy link
Copy Markdown
Member

Fixes #7071

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cah-sam-beran, cah-caleb-boyd, Dacrol, calebboyd, kachkaev, gotbahn, bradleyayers, g-plane, ksaldana1, ORESoftware, and 2 more reacted with thumbs up emoji cah-sam-beran, cah-caleb-boyd, Dacrol, calebboyd, tuxracer, GabrielCastro, paambaati, and ccstr reacted with hooray emoji kachkaev, s-ve, saadq, GabrielCastro, and paambaati reacted with heart emoji
All reactions
  • 👍 12 reactions
  • 🎉 8 reactions
  • ❤️ 5 reactions
Sheetal Nandi (sheetalkamat) added 10 commits February 23, 2018 13:54
@sheetalkamat
Resolve Json file when module resolution strategy is node
5771adb
@sheetalkamat
Replace usage of jsonObject on JsonSourceFile
ee2e267
@sheetalkamat
Bind and resolve the module for json file
3572fad
@sheetalkamat
Fix emit for json file
00b6c32
@sheetalkamat
More tests and also do not add index signature for json module
a1922fd
@sheetalkamat
Need allowJs to be true to use the json module resolution
ca590d6
@sheetalkamat
Parse all json values at root
a790a92
@sheetalkamat
Resolve the .json file only if module name contains the extension
4257c2f
@sheetalkamat
Report errors about correctness of the json file
23a7e2f
@sheetalkamat
Verify the output file paths for the json module emit
4f1640d
@sheetalkamat
Sheetal Nandi (sheetalkamat) requested a review from Mohamed Hegazy (mhegazy) February 24, 2018 18:21
@sheetalkamat Sheetal Nandi (sheetalkamat) mentioned this pull request Feb 24, 2018
Support importing json files as modules with commonjs or node module resolution #13665
Closed
@sheetalkamat
Ensure our readonly emptyArray stays non modified.
c154b81
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented Feb 26, 2018

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) c154b81 is the issue that was causing emptyArray to modify and my tests to fail.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@calebboyd

Caleb Boyd (calebboyd) commented Mar 8, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sheetalkamat
Merge branch 'master' into requireJson
2071466
@sheetalkamat
Sheetal Nandi (sheetalkamat) force-pushed the requireJson branch from 256ef2a to 2071466 Compare March 8, 2018 21:25
@sheetalkamat
Merge branch 'master' into requireJson
2d9af0b
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented Mar 10, 2018

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) can you please take a look so we can get this in. Thank you.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sheetalkamat Sheetal Nandi (sheetalkamat) mentioned this pull request Mar 11, 2018
Enable the commented out test and convert folder entries to sorted list #22450
Merged
Sheetal Nandi (sheetalkamat) added 2 commits March 12, 2018 16:22
@sheetalkamat
Merge branch 'master' into requireJson
28a988d
@sheetalkamat
Merge branch 'master' into requireJson
b4e0062
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Mar 31, 2018
Design Meeting Notes, 3/31/2018 #23045
Closed
Sheetal Nandi (sheetalkamat) added 4 commits April 5, 2018 15:18
@sheetalkamat
Merge branch 'master' into requireJson
9f72415
@sheetalkamat
Resolve json modules only when --resolveJsonModule is specified
a143963
@sheetalkamat
Reverted unintentional formatting changes
9d3ad54
@sheetalkamat
Merge branch 'master' into requireJson
ce08af4
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Apr 26, 2018
In JS, importing JSON should not give an error #23712
Closed
sandersn
Nathan Shively-Sanders (sandersn) reviewed Apr 26, 2018
View reviewed changes

@sandersn Nathan Shively-Sanders (sandersn) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sandersn Nathan Shively-Sanders (sandersn) mentioned this pull request Apr 26, 2018
Add prettier user test and fix associated crash #23715
Merged
sandersn
Nathan Shively-Sanders (sandersn) requested changes Apr 26, 2018
View reviewed changes

@sandersn Nathan Shively-Sanders (sandersn) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/commandLineParser.ts
parentOption: string | undefined
): any {
const result: any = {};
const result: any = returnValue ? {} : undefined;

@sandersn Nathan Shively-Sanders (sandersn) Apr 26, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 30, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sandersn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/parser.ts Outdated
// 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;

@sandersn Nathan Shively-Sanders (sandersn) Apr 26, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sheetalkamat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/compiler/parser.ts
else {
const statement = createNode(SyntaxKind.ExpressionStatement) as JsonObjectExpressionStatement;
switch (token()) {
case SyntaxKind.OpenBracketToken:

@sandersn Nathan Shively-Sanders (sandersn) Apr 26, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Apr 26, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 30, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Apr 30, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/program.ts
@@ -1983,7 +1983,7 @@ namespace ts {
}

@sandersn Nathan Shively-Sanders (sandersn) Apr 26, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 30, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

what?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Apr 30, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/types.ts

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

@sandersn Nathan Shively-Sanders (sandersn) Apr 26, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented Apr 30, 2018

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sheetal Nandi (sheetalkamat) added 2 commits April 30, 2018 11:44
@sheetalkamat
Merge branch 'master' into requireJson
579748b
@sheetalkamat
PR feedback
c7b2d92
sandersn
Nathan Shively-Sanders (sandersn) approved these changes Apr 30, 2018
View reviewed changes

@sandersn Nathan Shively-Sanders (sandersn) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/parser.ts
else {
const statement = createNode(SyntaxKind.ExpressionStatement) as JsonObjectExpressionStatement;
switch (token()) {
case SyntaxKind.OpenBracketToken:

@sandersn Nathan Shively-Sanders (sandersn) Apr 30, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/program.ts
@@ -1983,7 +1983,7 @@ namespace ts {
}

@sandersn Nathan Shively-Sanders (sandersn) Apr 30, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Apr 30, 2018
View reviewed changes
Comment thread src/compiler/emitter.ts
function emitExpressionStatement(node: ExpressionStatement) {
emitExpression(node.expression);
writeSemicolon();
if (!isJsonSourceFile(currentSourceFile)) {

@mhegazy Mohamed Hegazy (mhegazy) Apr 30, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) May 1, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) May 1, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

i see

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Apr 30, 2018
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
return links.type = anyType;
}
// Handle export default expressions
if (declaration.kind === SyntaxKind.SourceFile) {

@mhegazy Mohamed Hegazy (mhegazy) Apr 30, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Apr 30, 2018
View reviewed changes
Comment thread src/compiler/types.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

@mhegazy Mohamed Hegazy (mhegazy) Apr 30, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) Apr 30, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

or Json for consistency

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 30, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) May 1, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

sure..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhegazy

Mohamed Hegazy (mhegazy) commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

Andy (Andrewkraft) (@Andy-MS) can you please review this change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhegazy
Mohamed Hegazy (mhegazy) requested a review from a user April 30, 2018 23:12
@mhegazy

Mohamed Hegazy (mhegazy) commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

Sheetal Nandi (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sheetalkamat
PR feedback
97df079
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented May 3, 2018

Copy link
Copy Markdown
Member Author

chatted with Mohamed Hegazy (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sheetalkamat
Merge branch 'master' into requireJson
15f9ea3
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented May 3, 2018

Copy link
Copy Markdown
Member Author

Ping Andy (Andrewkraft) (@Andy-MS) Mohamed Hegazy (@mhegazy) for final review before merge
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ghost
Deleted user (ghost) reviewed May 4, 2018
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
// Handle export default expressions
if (isSourceFile(declaration)) {
Debug.assert(isJsonSourceFile(declaration));
const jsonSourceFile = <JsonSourceFile>declaration;

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/diagnosticMessages.json Outdated
"code": 6196,
"reportsUnnecessary": true
},
"Resolve module name imported with '.json' extension to the json source file.": {

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sandersn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@sheetalkamat Sheetal Nandi (sheetalkamat) May 4, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) May 4, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/commandLineParser.ts
parentOption: string | undefined
): any {
const result: any = {};
const result: any = returnValue ? {} : undefined;

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/moduleNameResolver.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) {

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) May 4, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ghost ghost mentioned this pull request May 4, 2018
Simplify getOutputExtension #23889
Closed
ghost
Deleted user (ghost) reviewed May 4, 2018
View reviewed changes
Comment thread src/compiler/program.ts
case Extension.Ts:
case Extension.Dts:
case Extension.Json:
// These are always allowed.

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/utilities.ts Outdated
}
}

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

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: StringLiteral | undefined

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/utilities.ts Outdated

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

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/harness/harness.ts

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));

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why filter these?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) May 4, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/services/getEditsForFileRename.ts
if (!cfg) return;
const oldFile = cfg.jsonObject && getFilesEntry(cfg.jsonObject, oldFilePath);
const configFile = program.getCompilerOptions().configFile;
const oldFile = getTsConfigPropArrayElementValue(configFile, "files", oldFilePath);

@ghost Deleted user (ghost) May 4, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Sheetal Nandi (sheetalkamat) added 2 commits May 4, 2018 10:45
@sheetalkamat
PR feedback
4e6586d
@sheetalkamat
Update the message for resolveJsonModule as per PR feedback
c4143ae
ghost
Deleted user (ghost) approved these changes May 4, 2018
View reviewed changes
mhegazy
Mohamed Hegazy (mhegazy) reviewed May 4, 2018
View reviewed changes
Comment thread src/compiler/program.ts
}

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

@mhegazy Mohamed Hegazy (mhegazy) May 4, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) May 4, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) approved these changes May 4, 2018
View reviewed changes

@mhegazy Mohamed Hegazy (mhegazy) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request May 4, 2018
Support new flags (2.9) #23627
Closed
4 tasks done
@sheetalkamat
Sheetal Nandi (sheetalkamat) merged commit 5133c70 into master May 4, 2018
@sheetalkamat
Sheetal Nandi (sheetalkamat) deleted the requireJson branch May 4, 2018 19:42
This was referenced May 4, 2018
Declare a type with a JSON file instead of an interface #23898
Closed
No completions for .json files with --resolveJsonModule #23899
Closed
Update schema for TS 2.9 flags SchemaStore/schemastore#451
Merged
@microsoft 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.

Reviewers

@sandersn Nathan Shively-Sanders (sandersn) sandersn approved these changes
+2 more reviewers
@mhegazy Mohamed Hegazy (mhegazy) mhegazy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@sheetalkamat @calebboyd @mhegazy @sandersn
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.