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

Add Compileroption to add Extensions to relative imports #47436

Closed
wants to merge 3 commits into from

Conversation

jogibear9988
Copy link

@jogibear9988 jogibear9988 commented Jan 14, 2022

@jogibear9988
Copy link
Author

also #16577

@jogibear9988
Copy link
Author

With this compiler option, valid typescript, could emit valid javascript for the browser. Otherwise the imports in browser are invalid.

@RyanCavanaugh
Copy link
Member

#15479 (comment)

@RyanCavanaugh
Copy link
Member

Please see https://github.com/microsoft/TypeScript/blob/main/CONTRIBUTING.md#contributing-features

@jogibear9988
Copy link
Author

@RyanCavanaugh
would a compiler option be accepted, where imports fail when they don't have the correct extension?

@jogibear9988
Copy link
Author

and one more question:
would a generic extension point configuration be accepted, where I can hook into the compiler to modify AST?

@jogibear9988
Copy link
Author

at the 👍 people here, would it be a solution if i create a typescript fork, where this is possible?

@MartinJohns
Copy link
Contributor

would a generic extension point configuration be accepted, where I can hook into the compiler to modify AST?

There is limited support via #13764. For more general plugin support there is #16607.

would a compiler option be accepted, where imports fail when they don't have the correct extension?

That sounds like a great job for a linter.

@RyanCavanaugh
Copy link
Member

would a compiler option be accepted, where imports fail when they don't have the correct extension?

Please log a suggestion for this (one might exist already; please search)

would a generic extension point configuration be accepted, where I can hook into the compiler to modify AST?

This has complexity pretty far beyond where we could reasonably expect an external contributor to do this

@jogibear9988
Copy link
Author

I add the changes of this pull as a diff, cause maybe I'll delete it at some time from my Repo...

    diff --git a/src/compiler/transformers/module/esnextAnd2015.ts b/src/compiler/transformers/module/esnextAnd2015.ts
    index 5e755e5083cb..d5e59f1ca1ac 100644
    --- a/src/compiler/transformers/module/esnextAnd2015.ts
    +++ b/src/compiler/transformers/module/esnextAnd2015.ts
    @@ -73,6 +73,10 @@ namespace ts {
                         // To give easy access to a synchronous `require` in node-flavor esm. We do the transform even in scenarios where we error, but `import.meta.url`
                         // is available, just because the output is reasonable for a node-like runtime.
                         return getEmitScriptTarget(compilerOptions) >= ModuleKind.ES2020 ? visitImportEqualsDeclaration(node as ImportEqualsDeclaration) : undefined;
    +                case SyntaxKind.ImportDeclaration:
    +                    return visitImportDeclaration(node as ImportDeclaration);
    +                case SyntaxKind.CallExpression:
    +                    return visitCallExpression(node as CallExpression);
                     case SyntaxKind.ExportAssignment:
                         return visitExportAssignment(node as ExportAssignment);
                     case SyntaxKind.ExportDeclaration:
    @@ -183,12 +187,85 @@ namespace ts {
                 return statements;
             }
     
    +        function appendModuleExtensionWhenNeeded(path: string) {
    +            if (path.startsWith('.') || path.startsWith('/')) {
    +                if (path.endsWith('.ts')) {
    +                    return path.slice(0, -2) + compilerOptions.appendModuleExtension;
    +                } else if (!path.endsWith('.js') &&
    +                    !path.endsWith('.mjs') &&
    +                    !path.endsWith('.json') &&
    +                    !path.endsWith('.css') &&
    +                    !path.endsWith('.' + compilerOptions.appendModuleExtension)) {
    +                    return path + '.' + compilerOptions.appendModuleExtension;
    +                }
    +            }
    +            return path;
    +        }
    +
    +        function visitImportDeclaration(node: ImportDeclaration): VisitResult<ImportDeclaration> {
    +            // append `.js` (or another extension specified in options) to relative imports
    +            if (getAppendModuleExtension(compilerOptions))
    +            {
    +                if (isStringLiteral(node.moduleSpecifier)) {
    +                    const newPath = appendModuleExtensionWhenNeeded(node.moduleSpecifier.text);
    +                    if (node.moduleSpecifier.text !== newPath) {
    +                        return factory.createImportDeclaration(
    +                            node.decorators,
    +                            node.modifiers,
    +                            node.importClause,
    +                            factory.createStringLiteral(newPath, node.moduleSpecifier.singleQuote),
    +                            node.assertClause);
    +                    }
    +                }
    +            }
    +            return node;
    +        }
    +
    +        function visitCallExpression(node: CallExpression): VisitResult<CallExpression> {
    +            // append `.js` (or another extension specified in options) to relative imports
    +            if (getAppendModuleExtension(compilerOptions))
    +            {
    +                if (isImportKeyword(node.expression)) {
    +                    if (node.arguments.length && isStringLiteral(node.arguments[0])) {
    +                        const pathLiteral = node.arguments[0];
    +                        const newPath = appendModuleExtensionWhenNeeded(pathLiteral.text);
    +                        if (pathLiteral.text !== newPath) {
    +                            const newArgs = [factory.createStringLiteral(newPath, pathLiteral.singleQuote), ...node.arguments.slice(1)];
    +                            node = factory.createCallExpression(
    +                                node.expression,
    +                                node.typeArguments,
    +                                newArgs)
    +
    +                        }
    +                    }
    +                }
    +            }
    +            return node;
    +        }
    +
             function visitExportAssignment(node: ExportAssignment): VisitResult<ExportAssignment> {
                 // Elide `export=` as it is not legal with --module ES6
                 return node.isExportEquals ? undefined : node;
             }
     
             function visitExportDeclaration(node: ExportDeclaration) {
    +            // append `.js` (or another extension specified in options) to relative exports
    +            if (getAppendModuleExtension(compilerOptions))
    +            {
    +                if (node.moduleSpecifier && isStringLiteral(node.moduleSpecifier)) {
    +                    const newPath = appendModuleExtensionWhenNeeded(node.moduleSpecifier.text);
    +                    if (node.moduleSpecifier.text !== newPath) {
    +                        node = factory.createExportDeclaration(
    +                            node.decorators,
    +                            node.modifiers,
    +                            node.isTypeOnly,
    +                            node.exportClause,
    +                            factory.createStringLiteral(newPath, node.moduleSpecifier.singleQuote),
    +                            node.assertClause);
    +                    }
    +                }
    +            }
    +
                 // `export * as ns` only needs to be transformed in ES2015
                 if (compilerOptions.module !== undefined && compilerOptions.module > ModuleKind.ES2015) {
                     return node;
    diff --git a/src/compiler/types.ts b/src/compiler/types.ts
    index 389cf73c8822..c30a7ae6a3ef 100644
    --- a/src/compiler/types.ts
    +++ b/src/compiler/types.ts
    @@ -6099,6 +6099,7 @@ namespace ts {
             mapRoot?: string;
             maxNodeModuleJsDepth?: number;
             module?: ModuleKind;
    +        appendModuleExtension ?: 'js' | 'mjs';
             moduleResolution?: ModuleResolutionKind;
             newLine?: NewLineKind;
             noEmit?: boolean;
    diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
    index aa518d99bab7..6dde3bad808d 100644
    --- a/src/compiler/utilities.ts
    +++ b/src/compiler/utilities.ts
    @@ -6161,6 +6161,12 @@ namespace ts {
                 ScriptTarget.ES3;
         }
     
    +    export function getAppendModuleExtension(compilerOptions: {module?: CompilerOptions["module"], appendModuleExtension ?: CompilerOptions["appendModuleExtension"]}) {
    +        return compilerOptions.appendModuleExtension &&
    +            (compilerOptions.module === ModuleKind.Node12 ||
    +             compilerOptions.module === ModuleKind.NodeNext);
    +    }
    +
         export function getEmitModuleKind(compilerOptions: {module?: CompilerOptions["module"], target?: CompilerOptions["target"]}) {
             return typeof compilerOptions.module === "number" ?
                 compilerOptions.module :

@jogibear9988
Copy link
Author

jogibear9988 commented Jan 16, 2022

@RyanCavanaugh

would a generic extension point configuration be accepted, where I can hook into the compiler to modify AST?

This has complexity pretty far beyond where we could reasonably expect an external contributor to do this

I now dig a little bit deeper in the Typescript Sourcecode.

function getScriptTransformers(compilerOptions: CompilerOptions, customTransformers?: CustomTransformers, emitOnlyDtsFiles?: boolean) {

there are custom transformers already...
and now I found this project...
https://github.com/dropbox/ts-transform-import-path-rewrite/tree/main/examples/ttypescript
so my whole approach is already possible without a change to typescript.

so the extension point already exists. And I could convert my TS change to external transformer (I think it should work). I will test, and this maybe could be the solution for most of the people wich had this issue

@jogibear9988
Copy link
Author

@RyanCavanaugh
I thought now this would already be possible, but the transformers are not setable from outside.
Why not enableing something like in ttsc see here: https://github.com/cevek/ttypescript
So someone could use the Transform Extension Point from outside

@Real-Gecko
Copy link

It's really annoying to add .js to each of your imports, especially when they're added via auto import in VSCode.

@lloydjatkinson
Copy link

lloydjatkinson commented Jan 16, 2023

I am both surprised and disappointed that a clear flawed design (bug) can just continue to exist and PR's to fix it are being rejected by seemingly one stubborn TS team member.

The fact that #40878 has overwhelming support in favour of fixing this should be an indicator that something is wrong. Scripts like this: https://gist.github.com/silassare/cc3db860edeea07298cb03f64738c997#file-fix-esm-import-paths-js should not be necessary in 2023, let alone 2020 when #40878 was opened.

TypeScript and ES are supposed to bring the ecosystem forwards.

@jmrossy
Copy link

jmrossy commented Feb 3, 2023

Still hoping the TS team reconsider on this one. I understand the argument about not altering JS behavior, but adding file extensions (behind a flag, disabled by default) doesn't feel like a meaningful semantic change and would create huge value for developers!

Or at least add some clear documentation about how one should use Typescript to output ESM code that 'just works' with Node

@ernestostifano
Copy link

Hi @RyanCavanaugh,

After reading all the issues and related comments about this (including this one of yours), I wonder:

Wouldn't everyone be happy if we allowed using .ts in imports identifiers and transform them into .js?

I mean, if the problem is not transforming perfectly valid JS code, then import {something} from './some/file.ts' for sure is not perfectly valid JS code. So this could be added just as another TS abstraction.

Example:

import {something} from './some/file' -> import {something} from './some/file'
import {something} from './some/file.ts' -> import {something} from './some/file.js'

The problem with import {something} from './some/file.js' -> import {something} from './some/file.js' is that it makes no sense, as that file does not actually exist in the source directory. It is confusing for both humans and machines.

(#47188, #40878, #15479, #13422, #16577)

@OzPineapple
Copy link

Just woking with the tsc tool and having the same problem. Need to compile a bunch of ts files wicth depends on others ts files and have to past evething to js after testing making the coding slow.
Please reconsider this.

@RyanCavanaugh
Copy link
Member

Please see this comment re: extension rewriting.

@GervinFung
Copy link

Good evening everyone, how about giving ts-add-js-extension a try? Unlike most tools that use regex to manipulate text, or act as a tsc plugin to add file extension, this tool manipulate the output from Abstract Syntax Tree (AST) and rewrite modified code back to it, and is independent of typescript, therefore has better control over other tools and one thing worth mentioning, is that regex is not suitable to parse relative import/export statement and modify it, which is why most of the tools that use it are not comprehensive

About this tool

Pros

  1. Find whether you are importing index file, ie, find whether you are importing/exporting something.js or something/index.js
  2. Find whether you are import something.js or something.mjs, and append that extension accordingly

Cons

  1. Have to specify the outDir value as argument, because 1. There can be multiple tsconfig"something".json, 2. I'like this tool to run independent of tsc

(Heck, it can also run on JavaScript project if developers are lazy to specify the file extension)

Try it, it should be very helpful as it quite comprehensive. Appreciate any feedback

@staplespeter
Copy link

@GervinFung Thanks for the link buddy, and for the package, it worked great. This is my preferred solution as I also do not want my source files referencing build artifacts, where the use of .js extensions on imports breaks Jest tests.

@microsoft microsoft locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReReview the Issue : Compiled JavaScript import is missing file extension
10 participants