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

Added option to keep file extensions. #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stagas
Copy link

@stagas stagas commented Sep 21, 2023

Hey, thanks for the extension! Very useful, though I was trying out Deno and it requires specifiers to include the filename extension, so I added an option config.keepExtensions: boolean to do just that.

One thing to note, however, is that it will do a path.join(item.name, 'index.ts') when it is a folder, which can be a problem when it is an index.tsx, though I was unsure how to solve that cleanly, maybe you can help here if you need this.

It solves #11 :)

Cheers!

@espenja
Copy link

espenja commented Oct 8, 2023

Are you sure this solves #11 ?
The request is specifically for allowing the .js extension (not .ts) to be added to the filename when the package is targeting ESM.
Running node --loader ts-node/esm ./src/index.ts crashes if a barrel file contains an import statement ending in .ts (as well as crashing when the import statement only imports with the folder syntax)

@stagas
Copy link
Author

stagas commented Oct 9, 2023

Files ending in .ts work in deno and esbuild and it should work for ESM as it keeps all extensions although i haven't tested that scenario.

Make sure you have the "allowImportingTsExtensions": true, in tsconfig.json compilerOptions before testing with the node loaders.

let filePath: string;
if (keepExtensions) {
filePath =
item.type === "folder" ? path.join(item.name, "index.ts") : item.name;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is this issue it will default to index.ts if it's importing a folder in the barrel, so perhaps somehow it needs to work for .js and other extensions. Not sure what is the right solution here, maybe a configuration or else check the file system and try all extensions until one succeeds. There might be a lib for that.

@espenja
Copy link

espenja commented Oct 9, 2023

Files ending in .ts work in deno and esbuild and it should work for ESM as it keeps all extensions although i haven't tested that scenario.

Make sure you have the "allowImportingTsExtensions": true, in tsconfig.json compilerOptions before testing with the node loaders.

I didn't know about that option, pretty neat!
Although this works for (some?) bundlers, I think it may add restrictions on how your tsconfig must be setup. For example, I don't believe this will work if you are using tsc to emit files.
From the docs of allowImportingTsExtensions:

This flag is only allowed when --noEmit or --emitDeclarationOnly is enabled, since these import paths would not be resolvable at runtime in JavaScript output files. The expectation here is that your resolver (e.g. your bundler, a runtime, or some other tool) is going to make these imports between .ts files work.

The intellisense in VSCode when hovering over that option also has additional requirements to nodeResolution, but I don't know which of these, or if both, are correct:

Allow imports to include TypeScript file extensions. Requires '--moduleResolution bundler' and either '--noEmit' or '--emitDeclarationOnly' to be set.

Maybe the config option could be changed to either keep extension or allow for more customizability by allowing the user to prefer a specific file extension being used?

@0x80
Copy link

0x80 commented Nov 21, 2023

I am a little confused. Why would we want the actual file extension instead of just supporting the ESM standard?

Deno uses ESM and although it might allow for .ts to be used in imports, it should work with .js because that's what ESM requires.

So the problem to solve I think is "make this vscode extension compatible with the ESM standard".

Besides appending .js it should also resolve imports to /index.js for sub-folders that have an index/barrel file.

Ideally, the solution should look at the closest package manifest, and if "type": "module" is set, it should use the file extensions on import, because that is what defines an ES module. So there should be no need for an extra config option I think. But we could start with a configuration flag to keep it simple.

I have different priorities at the moment, but I will try to submit a PR for this in the near future.

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

Successfully merging this pull request may close these issues.

None yet

3 participants