Use jsonc to parse nls manifest (#838)

* remove pre-commit hooks

* use jsonc to parse manifests

fixes #833

* revert using jsonc for package.json

* Update src/test/fixtures/nls/package.json

Author: joaomoreno

package-lock.json

@@ -44,6 +44,7 @@
 		"commander": "^6.1.0",
 		"glob": "^7.0.6",
 		"hosted-git-info": "^4.0.2",
+		"jsonc-parser": "^3.2.0",
 		"leven": "^3.1.0",
 		"markdown-it": "^12.3.2",
 		"mime": "^1.3.4",
@@ -90,4 +91,4 @@
 		"watch-files": "src/**",
 		"spec": "src/test/**/*.ts"
 	}
-}
+}

package.json

@@ -23,6 +23,7 @@ import {
 import { detectYarn, getDependencies } from './npm';
 import * as GitHost from 'hosted-git-info';
 import parseSemver from 'parse-semver';
+import * as jsonc from 'jsonc-parser';
 
 const MinimatchOptions: minimatch.IOptions = { dot: true };
 
@@ -1323,7 +1324,8 @@ export function readManifest(cwd = process.cwd(), nls = true): Promise<Manifest>
 			try {
 				return Promise.resolve(JSON.parse(manifestStr));
 			} catch (e) {
-				return Promise.reject(`Error parsing 'package.json' manifest file: not a valid JSON file.`);
+				console.error(`Error parsing 'package.json' manifest file: not a valid JSON file.`);
+				throw e;
 			}
 		})
 		.then(validateManifest);
@@ -1337,9 +1339,10 @@ export function readManifest(cwd = process.cwd(), nls = true): Promise<Manifest>
 		.catch<string>(err => (err.code !== 'ENOENT' ? Promise.reject(err) : Promise.resolve('{}')))
 		.then<ITranslations>(raw => {
 			try {
-				return Promise.resolve(JSON.parse(raw));
+				return Promise.resolve(jsonc.parse(raw));
 			} catch (e) {
-				return Promise.reject(`Error parsing JSON manifest translations file: ${manifestNLSPath}`);
+				console.error(`Error parsing JSON manifest translations file: ${manifestNLSPath}`);
+				throw e;
 			}
 		});
 

src/package.ts

@@ -90,7 +90,6 @@
 				"variables": {
 					"PickProcess": "extension.pickNodeProcess"
 				},
-				"aiKey": "AIF-d9b70cd4-b9f9-4d70-929b-a071c400b217",
 				"initialConfigurations": "extension.provideInitialConfigurations",
 				"configurationAttributes": {
 					"launch": {

src/test/fixtures/nls/package.json

@@ -3,6 +3,7 @@
 
 	"node.label": "Node.js",
 
+	// Example comment
 	"node.sourceMaps.description": "Use JavaScript source maps (if they exist).",
 	"node.outDir.description": "If source maps are enabled, the generated code is expected in this directory. Deprecated: use 'outFiles' attribute instead.",
 	"node.outFiles.description": "If source maps are enabled, these glob patterns specify the generated JavaScript files. If a pattern starts with '!' the files are excluded. If not specified, the generated code is expected in the same directory as its source.",

src/test/fixtures/nls/package.nls.json

@@ -23,6 +23,7 @@ import { spawnSync } from 'child_process';
 import { XMLManifest, parseXmlManifest, parseContentTypes } from '../xml';
 import { flatten } from '../util';
 import { validatePublisher } from '../validation';
+import * as jsonc from 'jsonc-parser';
 
 // don't warn in tests
 console.warn = () => null;
@@ -244,28 +245,26 @@ describe('collect', function () {
 });
 
 describe('readManifest', () => {
-	it('should patch NLS', () => {
+	it('should patch NLS', async function () {
 		const cwd = fixture('nls');
-		const raw = require(path.join(cwd, 'package.json'));
-		const translations = require(path.join(cwd, 'package.nls.json'));
+		const raw = JSON.parse(await fs.promises.readFile(path.join(cwd, 'package.json'), 'utf8'));
+		const translations = jsonc.parse(await fs.promises.readFile(path.join(cwd, 'package.nls.json'), 'utf8'));
+		const manifest = await readManifest(cwd);
 
-		return readManifest(cwd).then((manifest: any) => {
-			assert.strictEqual(manifest.name, raw.name);
-			assert.strictEqual(manifest.description, translations['extension.description']);
-			assert.strictEqual(manifest.contributes.debuggers[0].label, translations['node.label']);
-		});
+		assert.strictEqual(manifest.name, raw.name);
+		assert.strictEqual(manifest.description, translations['extension.description']);
+		assert.strictEqual(manifest.contributes!.debuggers[0].label, translations['node.label']);
 	});
 
-	it('should not patch NLS if required', () => {
+	it('should not patch NLS if required', async function () {
 		const cwd = fixture('nls');
-		const raw = require(path.join(cwd, 'package.json'));
-		const translations = require(path.join(cwd, 'package.nls.json'));
+		const raw = JSON.parse(await fs.promises.readFile(path.join(cwd, 'package.json'), 'utf8'));
+		const translations = jsonc.parse(await fs.promises.readFile(path.join(cwd, 'package.nls.json'), 'utf8'));
+		const manifest = await readManifest(cwd, false);
 
-		return readManifest(cwd, false).then((manifest: any) => {
-			assert.strictEqual(manifest.name, raw.name);
-			assert.notEqual(manifest.description, translations['extension.description']);
-			assert.notEqual(manifest.contributes.debuggers[0].label, translations['node.label']);
-		});
+		assert.strictEqual(manifest.name, raw.name);
+		assert.notStrictEqual(manifest.description, translations['extension.description']);
+		assert.notStrictEqual(manifest.contributes!.debuggers[0].label, translations['node.label']);
 	});
 });