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

Typescript extension mapping #108

Closed
scagood opened this issue Aug 19, 2023 · 6 comments
Closed

Typescript extension mapping #108

scagood opened this issue Aug 19, 2023 · 6 comments

Comments

@scagood
Copy link

scagood commented Aug 19, 2023

Prior issues:

Description:

In the tsconfig file you can change whether .tsx becomes .jsx, or .js. Currently (as far as I can see) only .tsx <-> .jsx is supported (this is the jsx: 'preserve' option (https://www.typescriptlang.org/tsconfig#jsx)

I would like to add a mecanism to specify what exension .tsx becomes.

Proposals

There are a couple of methods that come to mind:

  1. We could detect the tsconfig (using typescript, then automatically change the mapping.
import { dirname } from 'node:path';
import { sys, findConfigFile, readConfigFile, parseJsonConfigFileContent } from 'typescript';

const tsconfigPath = findConfigFile(filePath, sys.fileExists, 'tsconfig.json');
if (tsconfigPath == null) {
  // No ts, no mapping
  return {};
}
const tsconfigFile = readConfigFile(tsconfigPath, sys.readFile);
const config = parseJsonConfigFileContent(tsconfigFile.config, sys, dirname(tsconfigPath));

// Now we can check `config.options.jsx`
  1. We could detect the tsconfig (using get-tsconfig, then automatically change the mapping.
import { getTsconfig } from 'get-tsconfig';

const cache = new Map();
const { config } = getTsconfig(filePath, 'tsconfig.json', cache);

// Now we can check `config.options.jsx`
  1. We could provide a setting in the rules to configure the mappings, eg:
settings: {
  node: {
    typescriptExtensionMapping: {
      '.ts': '.js',
      '.cts': '.cjs',
      '.mts': '.mjs',
      '.tsx': '.jsx'
    }
  }
}

Additional

I am happy to work with (make a PR) for any of these options, or another if there is a better one!

Personally I prefer the last option as its the lightest weight, and if something changes in the future we don't need to worry immediatly as its just a userland config option.

@aladdin-add
Copy link

aladdin-add commented Aug 22, 2023

Thanks for the input! To me solution 3 seems to be the easiest and most reliable. But 1 or 2 also SGTM if the feature can be opt-in and not affect non-ts users.

question: will solution 1 work if the config was extending another file? e.g.

{
"extend": "@my-shared-tsconfig", // having the config here
}

@aladdin-add
Copy link

the option typescriptExtensionMapping can be also configured with the rule options, like node.versions: https://github.com/eslint-community/eslint-plugin-n#configured-nodejs-version-range

@scagood
Copy link
Author

scagood commented Aug 22, 2023

question: will solution 1 work if the config was extending another file? e.g.

🤔 I think it should as you need to pass the directory and ts's filesystem.

I know that option 2 does do that, and it has the added benefit of using fs caching too (which typescript directly does not seem to support)

the option typescriptExtensionMapping can be also configured with the rule options, like node.versions: eslint-community/eslint-plugin-n#configured-nodejs-version-range

This is kinda like what I was thinking:

  1. Rules config typescriptExtensionMapping
  2. Settings config typescriptExtensionMapping
    We could accept the jsx string here too, eg preserve, react, ...
    but that could cause issues if ts change, as we'd need to fix it here.
  3. tsconfig.json [compilerOptions.jsx] field
  4. { ".js": ".ts", ".cjs": ".cts", ".mjs": ".mts", ".jsx": ".tsx" }

One thing I have noticed is that for dealing with react the reverse mappings need to do a little more work. This is because js becomes both ts, and tsx, so something like this is also required:

{
  ".js": [ ".ts", ".tsx" ],
  ".cjs": [ ".cts" ],
  ".mjs": [ ".mts" ]
}
When testing locally a patch like this seemed to be required:
diff --git a/lib/util/check-existence.js b/lib/util/check-existence.js
index fac3f21abca96bf1751350b2c3cbfc47164e19bd..083d4978ca912edaef105e53545f6ca2d90b5752 100644
--- a/lib/util/check-existence.js
+++ b/lib/util/check-existence.js
@@ -32,15 +32,18 @@ exports.checkExistence = function checkExistence(context, targets) {
         let missingFile = target.moduleName == null && !exists(target.filePath)
         if (missingFile && isTypescript(context)) {
             const parsed = path.parse(target.filePath)
-            const reversedExt = mapTypescriptExtension(
+            const reversedExts = mapTypescriptExtension(
                 context,
                 target.filePath,
                 parsed.ext,
                 true
             )
-            const reversedPath =
-                path.resolve(parsed.dir, parsed.name) + reversedExt
-            missingFile = target.moduleName == null && !exists(reversedPath)
+            const reversedPaths = reversedExts.map(
+                reversedExt => path.resolve(parsed.dir, parsed.name) + reversedExt
+            )
+            missingFile = reversedPaths.every(
+                reversedPath => target.moduleName == null && !exists(reversedPath)
+            )
         }
 
         if (missingModule || missingFile) {
diff --git a/lib/util/map-typescript-extension.js b/lib/util/map-typescript-extension.js
index fe42800430c6aad5eeca52205f86f8d4698c226d..9a963938443838246dc87137a5cec8497b468be6 100644
--- a/lib/util/map-typescript-extension.js
+++ b/lib/util/map-typescript-extension.js
@@ -8,14 +8,13 @@ const mapping = {
     ".ts": ".js",
     ".cts": ".cjs",
     ".mts": ".mjs",
-    ".tsx": ".jsx",
+    ".tsx": ".js",
 }
 
 const reverseMapping = {
-    ".js": ".ts",
-    ".cjs": ".cts",
-    ".mjs": ".mts",
-    ".jsx": ".tsx",
+    ".js": [ ".ts", ".tsx" ],
+    ".cjs": [ ".cts" ],
+    ".mjs": [ ".mts" ],
 }
 
 /**
@@ -42,10 +41,12 @@ module.exports = function mapTypescriptExtension(
         if (isTypescript(context) && ext in reverseMapping) {
             return reverseMapping[ext]
         }
-    } else {
-        if (isTypescript(context) && ext in mapping) {
-            return mapping[ext]
-        }
+
+        return [ fallbackExtension ]
+    }
+
+    if (isTypescript(context) && ext in mapping) {
+        return mapping[ext]
     }
 
     return fallbackExtension

I think I should first make a seperate PR to handle the one to many extension issue, then come back to having a PR for changing the extensions via a setting. 👀

@aladdin-add
Copy link

PRs are welcome! 👍

@scagood
Copy link
Author

scagood commented Aug 22, 2023

After some testing its considerably faster to use get-tsconfig

Test code:
import { fileURLToPath } from 'node:url';
import { dirname } from 'node:path';
import typescript from 'typescript';

import { getTsconfig } from 'get-tsconfig';
const cache = new Map();

const JsxEmit = {
  0: 'none',
  1: 'preserve',
  2: 'react',
  3: 'react-native',
  4: 'react-jsx',
  5: 'react-jsxdev',
};

function getTypescriptJSX(filePath: string) {
  try {
    // const typescript = require('typescript').default;
    const { sys, readConfigFile, parseJsonConfigFileContent } = typescript;

    const tsconfigPath = typescript.findConfigFile(
      filePath,
      sys.fileExists,
      'tsconfig.json',
    );

    if (tsconfigPath != null) {
      const tsconfigFile = readConfigFile(tsconfigPath, sys.readFile);
      const config = parseJsonConfigFileContent(tsconfigFile.config, sys, dirname(tsconfigPath));

      const jsx = config?.options?.jsx;
      return JsxEmit[jsx ?? 0];
    }
  } catch {
    return null;
  }
}

function getTSConfigJSX(filePath: string) {
  return getTsconfig(filePath, 'tsconfig.json', cache)?.config?.compilerOptions?.jsx;
}

function test(name: string, count: number, func: () => void) {
  const start = performance.now();
  for (let i = 0; i < count; i++) {
    func();
  }
  const end = performance.now();

  console.info(`${name} (${count})`, end - start);
}

const TYPESCRIPT = 'using typescript';
const GET_CONFIG = 'using get-tsconfig';

test(TYPESCRIPT, 10, () => getTypescriptJSX(fileURLToPath(import.meta.url)));
test(GET_CONFIG, 10, () => getTSConfigJSX(fileURLToPath(import.meta.url)));

test(TYPESCRIPT, 100, () => getTypescriptJSX(fileURLToPath(import.meta.url)));
test(GET_CONFIG, 100, () => getTSConfigJSX(fileURLToPath(import.meta.url)));

test(TYPESCRIPT, 500, () => getTypescriptJSX(fileURLToPath(import.meta.url)));
test(GET_CONFIG, 500, () => getTSConfigJSX(fileURLToPath(import.meta.url)));

test(TYPESCRIPT, 1000, () => getTypescriptJSX(fileURLToPath(import.meta.url)));
test(GET_CONFIG, 1000, () => getTSConfigJSX(fileURLToPath(import.meta.url)));

test(TYPESCRIPT, 10000, () => getTypescriptJSX(fileURLToPath(import.meta.url)));
test(GET_CONFIG, 10000, () => getTSConfigJSX(fileURLToPath(import.meta.url)));

// console.info(getTSX('/Users/scagood/github/gethooked.io/apps/gethooked-web/src/app/page.tsx'));

Test results:

using typescript (10) 16.075999975204468
using get-tsconfig (10) 2.48170804977417
using typescript (100) 50.811582922935486
using get-tsconfig (100) 5.45091700553894
using typescript (500) 161.4930419921875
using get-tsconfig (500) 11.340291976928711
using typescript (1000) 291.0885409116745
using get-tsconfig (1000) 18.019999980926514
using typescript (10000) 2795.298791050911
using get-tsconfig (10000) 159.38912498950958

I can confirm that both methods do correctly follow config inheritence.

I am about to head out, so will try to PR later today, or maybe tomorrow 😁

@scagood
Copy link
Author

scagood commented Sep 25, 2023

I think that is everything covered now?

Shall we close this @aladdin-add, or can you think of something else I need to do?

(If you also think this is done, feel free to close it)

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

No branches or pull requests

2 participants