nx icon indicating copy to clipboard operation
nx copied to clipboard

nx tries to parse JSONC files as JSON which causes commands to fail

Open Zamiell opened this issue 3 years ago • 9 comments

I created a "hello-world"-style NX repository using the following command:

npx create-nx-workspace isaacscript --preset=ts

After that, the only modification I made was to add more content to the extensions.json file.

After that, I ran the following command:

nx generate @nrwl/js:library --name=isaac-typescript-definitions --buildable --publishable --importPath="isaac-typescript-definitions"

It fails, with the following error message:

Cannot parse .vscode/extensions.json: ValueExpected in JSON at position 440

The "extensions.json" file is a JSONC file. JSONC is a format that supports comments and trailing commas. Thus, I believe that the error message is related to NX is trying to parse the JSONC file as a JSON file.

Since it is common for "extensions.json", "settings.json", and so on to contain JSONC features, this bug should be fixed!

Zamiell avatar May 19 '22 15:05 Zamiell

Upon further investigation, it seems that the problem is much worse than what I outlined in the OP. Building anything with NX also fails, because it isn't able to parse trailing commas in a tsconfig.json file.

For example, running:

nx build isaac-typescript-definitions

Gives the following error:

{
  stack: 'Error: ValueExpected in D:\\Repositories\\isaacscript\\tsconfig.base.json at position 530\n' +
    '    at parseJson (D:\\Repositories\\isaacscript\\node_modules\\nx\\src\\utils\\json.js:30:15)\n' +
    '    at readJsonFile (D:\\Repositories\\isaacscript\\node_modules\\nx\\src\\utils\\fileutils.js:26:37)\n' +
    '    at readRootTsConfig (D:\\Repositories\\isaacscript\\node_modules\\nx\\src\\project-graph\\build-project-graph.js:233:45)\n' +
    '    at D:\\Repositories\\isaacscript\\node_modules\\nx\\src\\project-graph\\build-project-graph.js:40:30\n' +
    '    at Generator.next (<anonymous>)\n' +
    '    at D:\\Repositories\\isaacscript\\node_modules\\tslib\\tslib.js:118:75\n' +
    '    at new Promise (<anonymous>)\n' +
    '    at Object.__awaiter (D:\\Repositories\\isaacscript\\node_modules\\tslib\\tslib.js:114:16)\n' +
    '    at buildProjectGraphUsingProjectFileMap (D:\\Repositories\\isaacscript\\node_modules\\nx\\src\\project-graph\\build-project-graph.js:34:20)\n' +
    '    at D:\\Repositories\\isaacscript\\node_modules\\nx\\src\\daemon\\server\\project-graph-incremental-recomputation.js:145:126',
  message: 'ValueExpected in D:\\Repositories\\isaacscript\\tsconfig.base.json at position 530\n' +
    '\n' +
    'Because of the error the Nx daemon process has exited. The next Nx command is going to restart the daemon process.\n' +
    'If the error persists, please run "nx reset".'
}

(Position 530 is the trailing comma.)

Zamiell avatar May 19 '22 16:05 Zamiell

I just concluded the same as @Zamiell has mentioned with trailing comma in a tsconfig.json file.

To give some context to my issue, it failed from the 3rd paths entry due to the comma in the array element:

{
    ...
    "baseUrl": ".",
    "paths": {
      "@nx-stencil-webpack5/core-components": [
        "dist/libs/core-components"
      ],
      "@nx-stencil-webpack5/core-components/loader": [
        "dist/libs/core-components/loader"
      ],
      "@nx-stencil-webpack5/core-components/*": [
        "dist/libs/core-components/dist/components/*",
      ],
      "@nx-stencil-webpack5/core-components-angular": [
        "libs/core-components-angular/src/index.ts"
      ]
    }
  }
  "exclude": ["node_modules", "tmp"]
  ...

marckassay avatar May 29 '22 19:05 marckassay

It is actually parsing tsconfig files using the jsonc library, but tsconfig files are NOT jsonc. They're actually their own special, unique format. https://github.com/microsoft/TypeScript/issues/20384

I took a stab at a PR a while ago for another issue, but there are actually quite a few places throughout the codebase that read tsconfig files for various reasons, and the implementation will vary. The root of the solution involves the TS compiler api, as they expose a number of functions for this. If you just want to parse the raw file, this is pretty trivial:

import ts from 'typescript';

const parseTsConfig = (file: string) => {
  return ts.readConfigFile(file, ts.sys.readFile)
}

This return an object with either a config or an error property, so adding error handling to the above:

import ts, { Diagnostic } from 'typescript';

const readTsConfig = () => {
  const { config, error } = ts.readConfigFile(file, ts.sys.readFile);

  if (error) {
    // error object is a Diagnostic:
    // https://github.com/microsoft/TypeScript/blob/4f29633934c5ba1944ee9beb4422f1f7f4ea37c0/lib/typescript.d.ts#L2778-L2784
    throw new Error('could not read config');
  }
  return config;
};

However, depending on the use case, this could be insufficiently sophisticated, because it does not do anything to resolve the hierarchy created by extends in configs. If you need the merged options, it's a little trickier, but not too bad:

import ts, { Diagnostic, ParsedCommandLine } from 'typescript';

const readTsConfig = (file: string): any => {
  const content = ts.readConfigFile(file, ts.sys.readFile);
  const error: Diagnostic | undefined = content.error;

  if (error) {
    throw new Error('could not read config');
  }
  return content.config;
};

const resolveTsConfig = (file: string): ParsedCommandLine => {
  const config = readTsConfig(file);
  const basePath = path.dirname(file);
  return ts.parseJsonConfigFileContent(config, ts.sys, basePath);
};

This returns a different shape of object: https://github.com/microsoft/TypeScript/blob/4f29633934c5ba1944ee9beb4422f1f7f4ea37c0/lib/typescript.d.ts#L3024-L3034

The notable fields are raw, which is the content of the current file merged with some default values. The other is options, which is the combined set of options, including some normalization like absolute paths for rootDir, baseUrl, etc.

ehaynes99 avatar Jul 31 '22 22:07 ehaynes99

Thanks for the research, ehaynes99. That's good info.

there are actually quite a few places throughout the codebase that read tsconfig files for various reasons, and the implementation will vary

Sounds like there should just be a unified "getTSConfig()" helper function to be used throughout the repository, that properly parses it with the extends and so on.

(Note that we also need to parse settings.json and extensions.json as JSON-C as well.)

Zamiell avatar Jul 31 '22 22:07 Zamiell

Yeah, I saw a lot of things going through here: https://github.com/nrwl/nx/blob/master/packages/nx/src/utils/json.ts

I would lean toward a separate file next to it rather that functions within it just to make a clear distinction.

The main things to consider:

  • the ParsedCommandLine has things in different arrangements than the tsconfig schema
  • if there were 2 functions, one that parses the whole resolved hierarchy, and one that doesn't, they would be in different formats
  • since any existing parsing is doing an unchecked cast from any somewhere, it'll need extra care to track down the things that need to change
  • some existing code might not be expecting the full resolved chain or absolute paths. It's likely always what you want, but would need to make sure nothing was trying to work around the fact that it previously wasn't

ehaynes99 avatar Aug 01 '22 02:08 ehaynes99

I just ran into this issue too. They are using the jsonc-parser, which has an option for allowTrailingComma (I don't think jsonc allows trailing commas by default). The options are passed as the 3rd parameter here:

https://github.com/nrwl/nx/blob/master/packages/nx/src/utils/json.ts#L49

It would be nice if trailing commas were allowed by default, especially when reading tsconfig files.

DesignByOnyx avatar Aug 02 '22 20:08 DesignByOnyx

I was so excited to upgrade our monorepo to latest lerna + nx, only to run into this wall :(

theo-staizen avatar Aug 04 '22 07:08 theo-staizen

I was so excited to upgrade our monorepo to latest lerna + nx, only to run into this wall :(

I mean, it's easy enough to avoid by taking the trailing commas out of tsconfig files. I don't love it either, but not a deal breaker for a far better monorepo experience.

If you use vscode, the prettier formatter will strip them as a workaround:

 --- a/settings.json
+++ b/settings.json
@@ -1,6 +1,6 @@
 {
   "[jsonc]": {
-    "editor.defaultFormatter": "vscode.json-language-features"
+    "editor.defaultFormatter": "esbenp.prettier-vscode"
   },

ehaynes99 avatar Aug 04 '22 23:08 ehaynes99

What sucks about this issue is that you run into very early. I had just set up my Nx workspace, made a couple changes to migrate existing/working tsconfigs, and then tried to run my first Nx command, and it failed with a really obscure error (see OP). When I visited the file, there were no visible errors (my editor shows errors in tsconfigs). This was not a pleasant "first taste" of Nx, and many others are likely to run into it. It's an important issue because it affects new adopters of Nx, especially since Nx sells itself as "works with any project".

DesignByOnyx avatar Aug 05 '22 17:08 DesignByOnyx

Just came here to say that I ran into this after upgrading our application from Angular v14.1.3 to Angular v14.2.0. No changes to tsconfig.json files, and before the update trailing commas weren't an issue, but after the upgrade, running ng lint throws the following error:

Error: PropertyNameExpected in /app/tsconfig.json at position 636

Removing trailing commas resolved the issue.

Tohnmeister avatar Aug 26 '22 09:08 Tohnmeister

@Zamiell pointed out below, tsconfig files should be parsed differently than JSONC files used in VSCode:

(Note that we also need to parse settings.json and extensions.json as JSON-C as well.)

I'd suggest splitting the fix for this issue in two features, one for each case.


For the JSONC, the approach might be creating utilities similar to the ones used in manipulating JSON. Something along the following lines. These would be used whenever vscode files are being parsed.

import { applyEdits, EditResult, parse, ParseOptions } from 'jsonc-parser';
import type { Tree } from '../tree';

 export function parseJsonc<T extends object = any>(
  tree: Tree,
  path: string,
  parseOptions?: ParseOptions
): T {
  if (!tree.exists(path)) {
    throw new Error(`Cannot find ${path}`);
  }
  try {
    const content = tree.read(path, 'utf-8');
    return parse(content, undefined, parseOptions);
  } catch (e) {
    throw new Error(`Cannot parse ${path}: ${e.message}`);
  }
}

export function updateJsonc<T extends object = any>(
  tree: Tree,
  path: string,
  updater: (content: string, parsed: T) => EditResult,
  parseOptions?: ParseOptions
): void {
  if (!tree.exists(path)) {
    throw new Error(`Cannot find ${path}`);
  }
  try {
    const content = tree.read(path, 'utf-8');
    const parsed = parse(content, undefined, parseOptions);

    const edits = updater(content, parsed);

    const modified = applyEdits(content, edits);
    tree.write(path, modified);
  } catch (e) {
    throw new Error(`Cannot modify ${path}: ${e.message}`);
  }
}

The only downside here is modifying JSONC files are not as straightforward as modifying JSON, since the edits must be done using jsonc-parser utilities.


@AgentEnder do you think the suggested approach is good to go? If positive, I can work on it.

JulioC avatar Nov 14 '22 03:11 JulioC

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs. If we missed this issue please reply to keep it active. Thanks for being a part of the Nx community! 🙏

github-actions[bot] avatar May 14 '23 00:05 github-actions[bot]

The issue is still relevant and is a somewhat annoying flaw in nx, especially since it affects brand-new users who are onboarding new existing monorepos.

Zamiell avatar May 14 '23 00:05 Zamiell

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs. If we missed this issue please reply to keep it active. Thanks for being a part of the Nx community! 🙏

github-actions[bot] avatar Nov 12 '23 00:11 github-actions[bot]

The issue is still relevant and is a somewhat annoying flaw in nx, especially since it affects brand-new users who are onboarding new existing monorepos.

Zamiell avatar Nov 12 '23 03:11 Zamiell

The original error for parsing JSONC files have been fixed. We've also moved TS/JS inspection (for building Nx graph) to Rust, so the original issue of reading tsconfig.base.json isn't relevant.

I tested this on the latest version of Nx, and could not reproduce. If you run into any problems, please open a new issue with reproduction steps. Thanks!

jaysoo avatar Jan 12 '24 16:01 jaysoo

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

github-actions[bot] avatar Feb 12 '24 00:02 github-actions[bot]