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

transform: remove ".ts" extension in output #5395

Closed
lukeed opened this issue Sep 2, 2024 · 17 comments · Fixed by #5399
Closed

transform: remove ".ts" extension in output #5395

lukeed opened this issue Sep 2, 2024 · 17 comments · Fixed by #5399
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Comments

@lukeed
Copy link
Contributor

lukeed commented Sep 2, 2024

Given that this is typically used for library/bundler transformations, the generated output shouldn't contain extensions in the output.

I suppose the jury can still be out in regards to JS extensions, but TS extensions should definitely not be there.

Input

// src/walk.ts
import { join } from 'node:path';
import * as walk from './walk.ts';
import type { Options } from './walk.ts';

export type { Options };

export function up(name: string, options?: Options): string | undefined {
  // ...
}

Output

// output/find.mjs
import { join } from "node:path";
import * as walk from "./walk.ts";
export function up(name, options) {
  // ...
}

// output/find.d.mts
import type { Options } from "./walk.ts";
export type { Options };
export declare function up(name: string, options?: Options): string | undefined;

Note that both the generate dts and JS still contains the walk.ts reference.

Desired

// output/find.mjs
import { join } from "node:path";
import * as walk from "./walk";
export function up(name, options) {
  // ...
}

// output/find.d.mts
import type { Options } from "./walk";
export type { Options };
export declare function up(name: string, options?: Options): string | undefined;

Because this particular case is a library, ./walk will be re-resolved to the correct "exports" condition when needed.

And arguably, any bundler (or even runtime) contexts that ingest an extension-less import will still be able to resolve the local file sibling.

@lukeed lukeed added the C-bug Category - Bug label Sep 2, 2024
@Boshen Boshen self-assigned this Sep 2, 2024
@Boshen Boshen added A-transformer Area - Transformer / Transpiler and removed C-bug Category - Bug labels Sep 2, 2024
@Boshen
Copy link
Member

Boshen commented Sep 2, 2024

Hi Luke, are we talking about a feature request for the up coming transformer https://www.npmjs.com/package/oxc-transform or something else?

@lukeed
Copy link
Contributor Author

lukeed commented Sep 2, 2024

Hey -- Yes, I'm using that package, but it's currently a bug and not a feature request. When transforming TS source to JS, the generated JS still has imports referencing *.ts files.

@Boshen
Copy link
Member

Boshen commented Sep 2, 2024

For reproduction:

import { transform } from "oxc-transform";

let sourceText = `
import { join } from 'node:path';
import * as walk from './walk.ts';
import type { Options } from './walk.ts';

export type { Options };

export function up(name: string, options?: Options): string | undefined {
  // ...
}
`
console.log(transform("asdf.ts", sourceText, {
  typescript: {
    onlyRemoveTypeImports: true,
    declaration: true,
  }
}))
{
  sourceText: 'import { join } from "node:path";\n' +
    'import * as walk from "./walk.ts";\n' +
    'export function up(name, options) {}\n',
  declaration: 'import type { Options } from "./walk.ts";\n' +
    'export type { Options };\n' +
    'export declare function up(name: string, options?: Options): string | undefined;\n',
  errors: []
}

@Boshen Boshen added the C-bug Category - Bug label Sep 2, 2024
@lukeed
Copy link
Contributor Author

lukeed commented Sep 2, 2024

(On an unrelated note, sourceText is kinda a funny name for generated output. Renaming it is obviously breaking, but I'd suggest something like contents or javascript/js because "source text" makes me think it's the TypeScript source.)

@Boshen
Copy link
Member

Boshen commented Sep 2, 2024

@Dunqing apparently we need to port https://babel.dev/docs/babel-preset-typescript#rewriteimportextensions

import { transformSync } from "@babel/core";

let sourceText = `
import { join } from 'node:path';
import * as walk from './walk.ts';
import type { Options } from './walk.ts';

export type { Options };

export function up(name: string, options?: Options): string | undefined {
  // ...
}
`

console.log(
  transformSync(sourceText, {
    presets: [["@babel/preset-typescript", { onlyRemoveTypeImports:true, rewriteImportExtensions:true }]],
    filename: 'script.ts',
  })
)
  code: "import { join } from 'node:path';\n" +
    'import * as walk from "./walk.js";\n' +
    'export function up(name, options) {\n' +
    '  // ...\n' +
    '}',

@Boshen Boshen added C-enhancement Category - New feature or request and removed C-bug Category - Bug labels Sep 2, 2024
@Boshen Boshen removed their assignment Sep 2, 2024
@Boshen
Copy link
Member

Boshen commented Sep 2, 2024

(On an unrelated note, sourceText is kinda a funny name for generated output. Renaming it is obviously breaking, but I'd suggest something like contents or javascript/js because "source text" makes me think it's the TypeScript source.)

We'll align everything with babel.

@lukeed
Copy link
Contributor Author

lukeed commented Sep 2, 2024

That "./walk.js" output is still incorrect/invalid. There needs to be no extension.

@Boshen
Copy link
Member

Boshen commented Sep 2, 2024

Interesting, I suppose we need to change the API to allow removal. I see people had to come up with more plugins to fix this problem https://www.npmjs.com/package/babel-plugin-replace-import-extension?activeTab=readme 🤯

@Boshen
Copy link
Member

Boshen commented Sep 2, 2024

rewriteImportExtensions: 'rewrite' | 'remove' | false

@lukeed
Copy link
Contributor Author

lukeed commented Sep 2, 2024

Well, babel predates TS and export conditions, which are the two main players in affecting path resolution. Babel also positions itself & wants to be the be-all, end-all... AKA, it assumes it's always the final build.

I didn't know that OXC is aligning with Babel. NGL that makes me really nervous

@Boshen
Copy link
Member

Boshen commented Sep 2, 2024

I didn't know that OXC is aligning with Babel

Our goal is not 100% compat, but we are currently aligning with Babel 8 to bootstrap ourself, otherwise we'll never ship. (Babel 8 is never going to be shipped isn't it ...)

We can always bend the API to allow current and future use cases.

Dunqing added a commit that referenced this issue Sep 3, 2024
…ion (#5399)

close: #5395

Babel only supports `rewrite`, we also support `remove`
@Boshen
Copy link
Member

Boshen commented Sep 3, 2024

The latest [email protected] includes the option rewriteImportExtensions: 'remove', but it's not declaration yet.

In:

let ret = transform("asdf.ts", sourceText, {
  typescript: {
    onlyRemoveTypeImports: true,
    rewriteImportExtensions: 'remove',
    declaration: true,
  }
});

console.log('Transformed:')
console.log(ret.code);

console.log('Declaration:')
console.log(ret.declaration);

Out:

Transformed:
import { join } from "node:path";
import * as walk from "./walk";
export function up(name, options) {}

Declaration:
import type { Options } from "./walk.ts";
export type { Options };
export declare function up(name: string, options?: Options): string | undefined;

@Boshen
Copy link
Member

Boshen commented Sep 3, 2024

Declarations shouldn't remove extensions right? Otherwise type resolution will fail.

@lukeed
Copy link
Contributor Author

lukeed commented Sep 3, 2024

They should be removed. If only publishing a build/ directory with the JS and DTS, the dts points to nothing if retains the *.ts import

@Dunqing
Copy link
Member

Dunqing commented Sep 3, 2024

They should be removed. If only publishing a build/ directory with the JS and DTS, the DTS points to nothing if retains the *.ts import

I don't fully understand why we need to remove them, I have tried the above example in tsc, and our output is the same as tsc's.

We are looking at your build script https://github.com/lukeed/empathic/blob/efc67f9c7fc5d2b0a6d748eeabc8a45d35c0a799/scripts/build.ts


Updated:

After looking at your script, I thought I knew why you needed to remove them; The transformed .d.ts file is named *.d.mts. So those .d.ts files don't exist anymore.

@lukeed
Copy link
Contributor Author

lukeed commented Mar 17, 2025

The rewriteImportExtensions option now works as configured/desired for source files; however, it doesnt apply to the generated declarations file.

Input

// src/example.ts
import { Provider, type MODEL } from './model.ts';

export async function foobar(): string { ... }

Output: rewriteImportExtensions: 'rewrite'

// out/example.js ✅
import { Provider } from './model.js';

export async function foobar() {
  // ...
}

// out/example.d.ts ❌
import type { MODEL } from "./model.ts"; // <<< path does not exist

export declare function foobar(): string;
export {};

Output: rewriteImportExtensions: 'remove'

// out/example.js ✅
import { Provider } from './model';

export async function foobar() {
  // ...
}

// out/example.d.ts ❌
import type { MODEL } from "./model.ts"; // <<< path does not exist

export declare function foobar(): string;
export {};

In both scenarios, the example.d.ts file's import paths are unchanged. This is/will always be invalid since the source .ts file will not exist post-transform.

While the generated d.ts content does match tsc output, it only works when the dts is placed as sibling files to the source ts files. And for a npm context, the source *.ts files would have to be published alongside the *.d.ts files to work, which honestly makes the *.d.ts files redundant/irrelevant, since all types/enums/func definitions are repeated.

Actually, this could create scenarios where more complex generics are broken because two copies of the complex type are loaded (1 from .ts and 1 from d.ts) and are sufficiently complex that tsc treats them as separate types, breaking narrowing, inference, etc.

@Dunqing
Copy link
Member

Dunqing commented Mar 18, 2025

That makes sense, thank you @lukeed feedback. Consider we have supported the rewriteImportExtension option in the transformer, so I think this issue is resolved. Let's follow #9838 to track isolatedDeclaration support

@Dunqing Dunqing closed this as completed Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants