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

Invalid code generation with --module=amd and --resolveJsonModule=true #25517

Closed
LeviticusMB opened this issue Jul 9, 2018 · 9 comments
Closed
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@LeviticusMB
Copy link

TypeScript Version: Version 3.0.0-dev.20180707

Search Terms: amd json

Code

  1. Init workspace
#!/bin/bash

npm init --yes
npm add requirejs typescript@next
mkdir src

cat > src/main.ts <<EOF
import data from './data.json'
console.log('data.json:', data)
EOF

cat > src/data.json <<EOF
[ "a", "json", "object" ]
EOF

cat > tsconfig-commonjs.json <<EOF
{
  "compilerOptions": {
    "esModuleInterop": true,
    "resolveJsonModule": true,
    "outDir": "./build",
    "module": "commonjs",
    "moduleResolution": "node"
  },
  "include": ["src"]
}
EOF

cat > tsconfig-amd.json <<EOF
{
  "compilerOptions": {
    "esModuleInterop": true,
    "resolveJsonModule": true,
    "outFile": "./build/bundle.js",
    "module": "amd",
    "moduleResolution": "node"
  },
  "include": ["src"]
}
EOF

npx tsc --version
npx tsc -p tsconfig-commonjs.json
npx tsc -p tsconfig-amd.json

cat > run-commonjs.js <<EOF
#!/usr/bin/env node
require('./build/main')
console.log('Done');
EOF

(echo '#!/usr/bin/env node'; cat node_modules/requirejs/require.js build/bundle.js -) > run-amd.js << EOF
requirejs(['main'], function (main) {
  console.log('Done');
})
EOF

chmod +x run-commonjs.js run-amd.js

#rm -rf build src node_modules package*.json run*.js tsconfig*.json
  1. Run ./run-commonjs.js
  2. Run ./run-amd.js

Expected behavior:

Both scripts output

data.json: [ 'a', 'json', 'object' ]
Done

Actual behavior:

Only the commonjs version works. The AMD version can be fixed by applying this patch:

--- run-amd.js	2018-07-09 18:44:56.000000000 +0200
+++ run-amd-fixed.js	2018-07-09 18:50:19.000000000 +0200
@@ -2147,7 +2147,7 @@
 var __importDefault = (this && this.__importDefault) || function (mod) {
     return (mod && mod.__esModule) ? mod : { "default": mod };
 };
-["a", "json", "object"]
+define("data", [], ["a", "json", "object"])
 define("main", ["require", "exports", "data"], function (require, exports, data_json_1) {
     "use strict";
     exports.__esModule = true;

As you can see, the compiler outputs the json data as-is, without wrapping it with the required module definition code. Note that --module=system generates the same plain json-object in the bundle.

Playground Link:

Related Issues:

@LeviticusMB
Copy link
Author

Oh come on. :(

@Cryrivers
Copy link

😂 imho the correct way to fix this is not to have a such error message, but make JSON Module resolution and generation work on all module systems. I think I could accept reporting errors in 3.0. But could you please not mark this as Fixed and put a Milestone, 3.0.1 or 3.1 both acceptable and have a complete fix in the future.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 12, 2018

I am sorry, but complete fix is in the eyes of the beholders.

AMD loaders do not automatically handle parsing and loading of JSON into objects, unlike the Node.js require(), therefore the most appropriate thing to do is what the TypeScript team have done and disallow it. You can't magically make something that TypeScript doesn't have control over, work. AMD loaders require a plugin to properly deal with loading of JSON files.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 12, 2018

To be clear, statically building the JSON into an AMD module would not be a good solution either, because the whole intent is to allow the structure of the JSON to be typed checked at design time, but allow the runtime to still be a JSON file. The fact that the compiler copies it on build to the out directory could actually be argued outside of what TypeScript should actually be doing. Bundlers should be choosing to statically build assets or transform them to other formats.

@LeviticusMB
Copy link
Author

Defining objects as modules in AMD is perfectly fine and works great, especially with the new --esModuleInterop flag in TypeScript.

The proper fix for this issue is what I did in the diff. Instead, typescript@next now emits errors on our current code base (which was working just fine until this commit), where we use something like the example below and actually do bundle up the (external) modules and their JSON objects ourselves (see #25149).

This issue was about when the JSON file is inside our project, not from node_modules.

#!/bin/bash

set -x
rm -rf node_modules package*.json import-json.ts bundle*js

npm init --yes
npm add typescript@2.9.2 libphonenumber-js@1.2.14

cat > 'import-json.ts' <<EOF
import phoneMetadata from 'libphonenumber-js/metadata.mobile.json';

console.log(phoneMetadata)
EOF

# tsc 2.9.2 works
npx tsc --esModuleInterop --resolveJsonModule --outFile bundle-2.9.2.js --module amd --moduleResolution node import-json.ts

# tsc 3.0.0 does not
npm add typescript@next
npx tsc --esModuleInterop --resolveJsonModule --outFile bundle-next.js --module amd --moduleResolution node import-json.ts

@Cryrivers
Copy link

actually my module setting is esnext. i guess in this case TypeScript should just type-check JSON files, not compile it (remaining as-is), and let bundlers to decide how to deal with them.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 12, 2018

ESNext represents ECMAScript standards track features. as of now, ES Modules only target code modules (i.e. JS). anything else like json, css, images, etc.. are all not allowed. TS is trailing the ECMAScript specification on this one.

@Cryrivers
Copy link

@mhegazy understood. thanks for the explanation.

@gamedevsam
Copy link

I just want vscode to type check JSON files for me, so sad x_x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

7 participants