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

Completion list for JSON files imported using CommonJS require #9221

Conversation

vojtechhabarta
Copy link

@vojtechhabarta vojtechhabarta commented Jun 16, 2016

Fixes #7071

This change provides completion lists for JSON files that are loaded using CommonJS require function.
For example for json1.json file

{
    "foo": "foo",
    "bar": "bar"
}

and test.ts file

var json1 = require("./json1");
json1. // <---

TypeScript compiler now provides completion list with foo and bar items.

Implementation

It is implemented as proposed in #7071 (comment) comment.

  • JSON files are parsed using TypeScript parser parseObjectLiteralExpression() function
  • node module resolver is extended to support .json extension
  • added ScriptKind.JSON

EDIT: removed description of my problem with tests (solved by updating dependencies using npm install)

@msftclas
Copy link

Hi @vojtechhabarta, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@mhegazy
Copy link
Contributor

mhegazy commented Jun 16, 2016

//cc @DanielRosenwasser who was looking into this earlier.

@piotrwitek
Copy link

+1

@mhegazy
Copy link
Contributor

mhegazy commented Jul 8, 2016

@vojtechhabarta sorry for the delay. I am a bit busy with TS 2.0 release at the moment, but i would love to get this change in once that is out.

@mhegazy mhegazy self-assigned this Sep 13, 2016
@aluanhaddad
Copy link
Contributor

It would be nice if this could be generalized. If the user writes an import statement like

import * as json1 from './json1';

or

import json1 from './json1'; // iff "allowSyntheticDefaultImports": true

it should work just as well. @mhegazy what do you think?

@vojtechhabarta
Copy link
Author

I wanted to resubmit this PR based on current master but I don't know how to solve one thing.

Recently #11370 was merged which adds following change: "Return type of require call is from external module resolution only if it doesn't resolve to local module" (checker.ts:12797 - https://github.com/Microsoft/TypeScript/pull/11370/files).
However this resolution is also used when there is node.d.ts in compilation so in this example test:

// @allowjs: true
// @outDir: dist
// @Filename: m1.js
export var a = 42;

// @Filename: m2.js
const m1 = require("./m1");
const a = m1.a;

// @Filename: node.d.ts
interface NodeRequireFunction {
    (id: string): any;
}
interface NodeRequire extends NodeRequireFunction {
}
declare var require: NodeRequire;

type of m1.a in any instead of number as it was before #11370.

Same applies for require with JSON file. I don't know how to distinguish between require from node.d.ts and other require cases.

@mhegazy, @sheetalkamat do you have any idea how this could be improved so it works also with node.d.ts?

@sheetalkamat
Copy link
Member

Checkout this #11819

@vojtechhabarta vojtechhabarta force-pushed the intellisenseForRequireJSON branch from 16ce1c1 to ac8edc6 Compare October 26, 2016 21:23
@vojtechhabarta
Copy link
Author

Thanks @sheetalkamat for the link, that's exactly what I needed.

@mhegazy I rebased this PR on current master branch.

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2017

closing in favor of #13665

@mhegazy mhegazy closed this May 22, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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.

In JS, importing JSON should not give an error
6 participants