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

Parsing tsconfig.json with support for extra commas #48

Closed
ZSkycat opened this issue Jun 19, 2018 · 16 comments · Fixed by #58
Closed

Parsing tsconfig.json with support for extra commas #48

ZSkycat opened this issue Jun 19, 2018 · 16 comments · Fixed by #58

Comments

@ZSkycat
Copy link

ZSkycat commented Jun 19, 2018

package.json

  "dependencies": {
    "ts-node": "^6.1.1",
    "tsconfig-paths": "^3.4.0",
    "typescript": "^2.9.2"
  },

run command ts-node -r tsconfig-paths/register.js test.ts

C:\Users\Cat\Desktop\toy-ts-node\node_modules\tsconfig-paths\lib\tsconfig-loader.js:72
    var config = JSON.parse(cleanedJson);
                      ^
SyntaxError: Unexpected token } in JSON at position 305
    at JSON.parse (<anonymous>)
    at loadTsconfig (C:\Users\Cat\Desktop\toy-ts-node\node_modules\tsconfig-paths\lib\tsconfig-loader.js:72:23)
    at loadSyncDefault (C:\Users\Cat\Desktop\toy-ts-node\node_modules\tsconfig-paths\lib\tsconfig-loader.js:27:18)
    at tsConfigLoader (C:\Users\Cat\Desktop\toy-ts-node\node_modules\tsconfig-paths\lib\tsconfig-loader.js:13:22)
    at Object.configLoader (C:\Users\Cat\Desktop\toy-ts-node\node_modules\tsconfig-paths\lib\config-loader.js:27:22)
    at Object.register (C:\Users\Cat\Desktop\toy-ts-node\node_modules\tsconfig-paths\lib\register.js:10:46)
    at Object.<anonymous> (C:\Users\Cat\Desktop\toy-ts-node\node_modules\tsconfig-paths\register.js:1:15)
    at Module._compile (internal/modules/cjs/loader.js:654:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:665:10)
    at Module.load (internal/modules/cjs/loader.js:566:32)

I really use the command
cross-env TS_NODE_PROJECT=./tool/tsconfig.json && ts-node -r tsconfig-paths/register ./node_modules/webpack/bin/webpack.js --config ./tool/config.webpack.dev.ts

cross-env TS_NODE_PROJECT=./tool/tsconfig.json && node -r tsconfig-paths/register webpack --config ./tool/config.webpack.dev.ts

I don't know how to use it.

@ZSkycat
Copy link
Author

ZSkycat commented Jun 20, 2018

I know why.because my tsconfig.json uses jsonc.
TypeStrong/ts-node#409 (comment)

But, tsc support jsonc. It would be better to have tsconfig-paths support jsonc.

{
    "compilerOptions": {
        "rootDir": ".",
        "outDir": "../dist/tool",
        "target": "esnext",
        "module": "commonjs",
        "moduleResolution": "node",
        "sourceMap": true,
        "strict": true,
        "types": ["node"],
    },
    "include": [
        "./**/*",
    ],
}

@jonaskello
Copy link
Member

Yes that makes sense. To put this in some context, first we used the tsconfig package to do parsing of tsconfig.json. Then we needed to support the extends syntax and the tsconfig package did not support that (at least at that point in time). So IIRC @Jontem switched to just parsing it as regular JSON, but first passing it through the strip-json-comments package, so comments should already be supported.

So given the above, I guess what is missing is support for extra commas, I would be happy to take a PR for adding this support (while still keeping the extends and comments support that already exists).

@jonaskello jonaskello changed the title ts-node -r tsconfig-paths/register.js can not work Parsing tsconfig.json with support for extra commas Jun 20, 2018
@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Jul 13, 2018

It looks like tsconfig currently uses JSON.parse, which would blow chunks on the trailing comma.

I haven't tried it yet, but Microsoft/node-jsonc-parser might be what the doctor ordered for parsing jsonc, and json5 for trailing commas (if node-jsonc-parser doesn't support that already).

@jonaskello
Copy link
Member

IIRC tsconfig-paths currently strips comments and then uses plain JSON.parse(). It would indeed be interesting to know what tsc uses internally to parse tsconfig.json.

@jonaskello
Copy link
Member

jonaskello commented Jul 16, 2018

I think json5 is the most popular choice if we should go with an external library. There seems to be a function called findConfigFile in the typescript API but not sure if that is useful for actually loading the tsconfig.json file.

UPDATE: Found another function called parseJsonConfigFileContent in the typescript API. However not sure that we want to take the typescript package as a dependency just to read tsconfig.json.

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Jul 27, 2018

@jonaskello make it a peerDependency? If someone is using tsconfig-paths, there's a very good (if not guaranteed?) chance s/he is already using typescript.

@jonaskello
Copy link
Member

jonaskello commented Jul 27, 2018

@jshado1 I was thinking the same thing at first. However some ppl use tsconfig-paths in production with something like node -r tsconfig-paths/register main.js. In that case they probably only install dependencies with npm install --productionwhile typescript is probably is a devDependency or peerDependency. So if we require the full typescript package to be installed then usage in production would also require typescript installed.

@JakobJingleheimer
Copy link
Contributor

I think the arguments from babel/register (babel/babel#5851) apply here: The register should not be used in production: It should be pre-compiled, which is actually how I'm trying to use it, paired with tspaths. The register is just for developer convenience during actual development (and running specs).

@jonaskello
Copy link
Member

jonaskello commented Jul 27, 2018

Yes, I would tend to agree with that and I do not use it in production myself or recommend it. Still I think it would be better to have a small dependency so we keep our options open.

From my research JSON5 seems like the most popular option so I think we should simply switch to use that instead of the built-in JSON.parse(). Initially we used the tsconfig package and that does a stripping of BOM with the strip-bom package. I'm not sure if JSON5 has this stripping built-in or if we still need to do that before parsing.

Anyway, if someone would like to provide a PR to change to JSON5 that would be accepted :-).

@jonaskello
Copy link
Member

Specifically I think this line needs to use JSON5.

JakobJingleheimer added a commit to JakobJingleheimer/tsconfig-paths that referenced this issue Jul 28, 2018
* Switch from JSON.parse to JSON5.parse

resolves dividab#48
@JakobJingleheimer
Copy link
Contributor

@jonaskello done 🙂

jonaskello pushed a commit that referenced this issue Jul 28, 2018
* Add support for comments and trailing commas

* Switch from JSON.parse to JSON5.parse

resolves #48

* add ts typings for json5
@JakobJingleheimer
Copy link
Contributor

By the way, @jonaskello how do you yourself handle this in production? Is tsconfig-paths able to alter the output to replace the paths?

@jonaskello
Copy link
Member

jonaskello commented Jul 28, 2018

I mostly do monorepos with yarn workspaces. There is a nice example of how to do it here. Notice how paths in tsconfig are used in development but not in production, as stated towards the end in that repos readme. Usually I have client, server, and shared package in the monorepo, sometimes more packages in order to split things more nicely. The client side is handled by using webpack with tsconfig-paths-webpack-plugin so the paths are resolved while bundling. For the server side normal npm package resolving works once all packages has been built (and yarn install is used on the workspace root so all my packages are symlinked under the common node_modules).

If you want to resolve the paths statically, I noticed there is the tspath package. If I understand the readme correctly it replaces the paths statically after building but I have not tried it.

@JakobJingleheimer
Copy link
Contributor

Thanks!

I was hoping to avoid adding a huge dependence like webpack for only 1 tiny feature.

I saw tspaths—the readme sounds exactly like what I want. Unfortunately, it seems to nbe broken :(

@jonaskello
Copy link
Member

jonaskello commented Jul 29, 2018

If you really want static path resolving I guess it should not be too hard to do a package that does that. AFAIK tsc does not have a plugin system yet so you would have to compile with tsc first and then replace the paths in the resulting js files.

The way I would do it would be to use the typescript or babel-parser package to parse the js files into an AST, and look for any import or require nodes. Then use the API in the tsconfig-path package (createMatchPath) to resolve the path for those node and replace them. Since other packages are doing the hard work (parsing and figuring out the new path) I guess it should be fairly simple to implement but of course there could be some pitfalls.

@jonaskello
Copy link
Member

Did some more research and it seems like you could also achieve static path replacement by writing a plugin to ttypescript. That would probably be the simplest and most integrated way available today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants