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

In JS, importing JSON should not give an error #7071

Closed
Thaina opened this issue Feb 13, 2016 · 14 comments
Closed

In JS, importing JSON should not give an error #7071

Thaina opened this issue Feb 13, 2016 · 14 comments
Assignees
Labels
Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue

Comments

@Thaina
Copy link

Thaina commented Feb 13, 2016

Make a json file

{
    "season": 1
}

Code

var test = require("test.json");

result

untitled-1

expect

untitled-2

@kitsonk
Copy link
Contributor

kitsonk commented Feb 13, 2016

This is the domain of plugin loaders and there are mechanism already for describing how a plugin loader would work and #6615 will provide a better mechanism for handling this.

Building this into TypeScript directly wouldn't make sense and likely break lots of other implementations, where people are properly using loader plugins.

@Thaina
Copy link
Author

Thaina commented Feb 13, 2016

@kitsonk Sorry but I'm not sure why it about plugin loader while this case is about intellisense (which I think it is provided by salsa)

@yortus
Copy link
Contributor

yortus commented Feb 13, 2016

The compiler could do this in theory, parsing the JSON structure in a similar way to how it would parse an object literal in a .ts or .js file. But as @kitsonk points out this is not a standardised language-level feature. Node.js allows you to require JSON files, but it's not part of any standard so it's unlikely to be implemented directly in TypeScript.

Having said that, JSX parsing is implemented directly in TypeScript even though that's not a standard. So it's probably more a question of demand and cost/benefit. If hundreds of +1s start appearing in this issue, who knows? I personally doubt that there's a lot of demand for this, and it's pretty easy to add your own typings for JSON documents anyway.

@Thaina
Copy link
Author

Thaina commented Feb 13, 2016

@yortus I think both require and module is also not standard js. It is standard of just for nodejs which also standardize load json file as module

I'm not sure how salsa implement, which part call which compiler, however salsa could read all those weird jsdoc and constructor to parse into intellisense so just include require json file should be in the same place as putting other intellisense

@billti
Copy link
Member

billti commented Feb 13, 2016

It would be doable with some work, and as commented, we already have some Node specific behavior. That said, as others have commented, this is a very little used feature, so unless we see a lot of demand for it, I don't see it reaching the top of our TODO list any time soon.

@billti billti added the Suggestion An idea for TypeScript label Feb 13, 2016
@kitsonk
Copy link
Contributor

kitsonk commented Feb 15, 2016

I am not 100% sure in Salsa, but isn't there a way to assert the an object literal type in JSDoc for this, something like:

/**
 * @type {Foo}
 */
var foo = require('foo.json');

In TypeScript that is generally what I do when loading JSON and while in theory the structures could get out of sync at design time, they of course can get out of sync at run-time too, so having them "automagically" determined at run-time is of somewhat limited value.

@RyanCavanaugh RyanCavanaugh added the In Discussion Not yet reached consensus label Feb 16, 2016
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Mar 15, 2016
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Mar 15, 2016
@RyanCavanaugh
Copy link
Member

Accepting PRs on this if anyone would like to give it a shot. This should only be allowed when the module system is commonjs

@RichiCoder1
Copy link

I'd be interested in taking a swing at this, but I haven't worked with Typescripts internals before. Any advice beyond the general contributing guidance?

@basarat
Copy link
Contributor

basarat commented Mar 15, 2016

Minor: that require statement should be most likely have a relative path prefix require('./test.js') because of how node modules work 🌹

@mhegazy
Copy link
Contributor

mhegazy commented Mar 15, 2016

I would start by looking at:
https://github.com/Microsoft/TypeScript/wiki/Architectural-Overview and https://github.com/Microsoft/TypeScript/wiki/Contributing-to-TypeScript

I would assume the first step is getting a JSON parser in place. i would expect that to be based on our current parser, possibly with an option to createSourceFile, or just a new ScriptKind.
We will need to then wire module resolution to accept .json files, along with other extensions.
Then wire the type system pieces to get the type of a JSON object, that should not be too hard.

@Thaina
Copy link
Author

Thaina commented Mar 15, 2016

Thanks for you all hard work

There is guideline from node people I think it would be useful

https://nodejs.org/api/modules.html#modules_all_together

@mischkl
Copy link

mischkl commented Apr 28, 2016

+1 it just feels natural to be able to do this if you're used to doing it within Node. Making it a built-in feature would ease the transition to TypeScript for Node-based projects.

Also, I could imagine this feature being useful in the browser as well - maybe TypeScript could pave the way for JSON importing in the next ECMAScript spec.

@piotrwitek
Copy link

+1

@zheeeng
Copy link

zheeeng commented Sep 25, 2017

+1 For this feature to benefits usage such as importing JSON configurations/locales to projects.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.