Skip to content

Add method host.createCustomSourceFile for custom file extensions #52983

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

Open
uasan opened this issue Feb 26, 2023 · 10 comments
Open

Add method host.createCustomSourceFile for custom file extensions #52983

uasan opened this issue Feb 26, 2023 · 10 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@uasan
Copy link

uasan commented Feb 26, 2023

Suggestion

Hello, I like the new option --allowArbitraryExtensions in 5 TS, but this only works for created files on the file system or overriding a set of fs methods (readDirectory , readFile , etc).

Generating files on real time is not very efficient, so we have to override different methods so that the TS receives the created instance SourceFile without creating a file on filesystem.

I propose to add a new method createCustomSourceFile to the host, which is undefined by default, if the user has defined this method in the host, then the TS should call it for all files with extensions that the TS does not support (.css, .svg, .vue, etc).

📃 Motivating Example

host.createCustomSourceFile = function (fileName, sourceText): SourceFile {
  switch (fileName.slice(fileName.lastIndexOf('.'))) {
    case '.css':
      return ts.createSourceFile(fileName, getDtsOfCss(sourceText));

    case '.svg':
      return ts.createSourceFile(fileName, getDtsOfSvg(sourceText));
  }
};

💻 Use Cases

  1. Bundle systems that always take care of importing files that TS doesn't support.
  2. Custom language servers.
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Mar 3, 2023
@RyanCavanaugh
Copy link
Member

@sheetalkamat any thoughts on this?

@uasan
Copy link
Author

uasan commented Mar 3, 2023

I'm almost sure without such a method, builders and language servers will be forced to redefine the internal interfaces of the TS, even in version 5.x where they made getters on all properties of the TS object, this is a really necessary functionality.

@sheetalkamat
Copy link
Member

I am not sure I understand the concern here,

By default, this is how the trace for resolving say ".css" extension looks like from "a.ts" where only "b.css" exists next to it.

======== Resolving module './b.css' from 'c:/temp/test3/a.ts'. ========
Module resolution kind is not specified, using 'Node10'.
Loading module as file / folder, candidate module location 'c:/temp/test3/b.css', target file types: TypeScript, Declaration.
File name 'c:/temp/test3/b.css' has a '.css' extension - stripping it.
File 'c:/temp/test3/b.d.css.ts' does not exist.
File 'c:/temp/test3/b.css.ts' does not exist.
File 'c:/temp/test3/b.css.tsx' does not exist.
File 'c:/temp/test3/b.css.d.ts' does not exist.
Directory 'c:/temp/test3/b.css' does not exist, skipping all lookups in it.
Loading module as file / folder, candidate module location 'c:/temp/test3/b.css', target file types: JavaScript.
File name 'c:/temp/test3/b.css' has a '.css' extension - stripping it.
File 'c:/temp/test3/b.css.js' does not exist.
File 'c:/temp/test3/b.css.jsx' does not exist.
Directory 'c:/temp/test3/b.css' does not exist, skipping all lookups in it.
======== Module name './b.css' was not resolved. ========

So we would never resolve module to file b.css as such. As shown above we are specifically looking for corresponding .d.ts files. Which means the host would need to update their fileExists implementations anyway to simulate presence of .dts file (if not on the disk) for the .css. So in addition to doing fileExists you would also need to override the readFile to get this text so I don't think you need custom source file creation method. Instead you would move the logic you have above to readFile. there is no other way to get around this if the "d,ts" doesnt exist.

@uasan
Copy link
Author

uasan commented Mar 3, 2023

@sheetalkamat How do you evaluate the design of such a solution, redefining fileExists, readFile, don't you think that this is at least a bad design that should be improved and everyone will benefit?

@uasan
Copy link
Author

uasan commented Mar 3, 2023

About performance, I assumed that checking for the presence of the createCustomSourceFile method in the host eliminates the need to check for the presence of a file in the file system, i.e. if there is a method, it will return the AST or undefine, which means that there is no file, yes, I can override the fs methods, but they are called a lot of times and my checks in these fs methods on a custom file extensions 99% will have a miss, this does not improve performance, not to mention the design of the solution itself.

@RyanCavanaugh
Copy link
Member

How do you evaluate the design of such a solution, redefining fileExists, readFile, don't you think that this is at least a bad design that should be improved and everyone will benefit?

@uasan this is very unconstructive. Of course we don't want to do bad things. The question is whether the proposal is bad or good (or bad or good relative to other options). Everyone agrees we should do better things once we agree on what the better thing is (and why). Framing it like "why not do better?" is begging the question.

@sheetalkamat
Copy link
Member

--allowArbitraryExtensions does not mean resolve module to arbitrary extension. It means that we will not error on these kinds of imports and look for corresponding d.ts files. it also does not mean we will mimic presence of ".d.ts" files.

The fileExists overrride on CompilerHost just needs to rewrite as:

const originalFileExists = host.fileExists.bind(host);
host.fileExists = fileName => {
   if (fileName.endsWith(".d.css.ts") fileName = fileName.substring(0, ".d.css.ts".length);
   return originalFileExists(fileName);
}

@uasan
Copy link
Author

uasan commented Mar 3, 2023

Ok, I understand your answer that you think this is a normal solution.

Is my feedback from the developer of build systems, I sincerely don’t understand why in order to integrate with the compiler I have to override filesystem methods, why there are no special methods for integrations, fs responsibility zone is only file handling system, it should not be responsible for integrating with compiler extensions, do not consider this as non-constructive trolling, I am sincerely surprised.

I can close this ticket.

@uasan
Copy link
Author

uasan commented Mar 4, 2023

I will also describe the reasons why we and many others do not create d.ts files on the file system, except for the performens, there is a difficulty in synchronizing the generation of files and scanning the TS file system, the fact is that the our builder do not know about exists files (for example, CSS), they learn about their presence from the TS compiler or do their own parallel scan of folders.

When a scan finds a CSS file, its contents are sent to the CSS parser get AST(CSS), then generate d.ts source text from AST(CSS) and saved to disk, writing to disk triggering a new scan of the TS that finds the d.ts file, as a result we get a lot of scans and only the last gives the final result, it is difficult, it is much more efficient not to create a d.ts file, but to generate their contents inside the TS process.

@uasan
Copy link
Author

uasan commented Mar 21, 2023

@sheetalkamat I did as you suggested, overridden fileExists and readFile on allowArbitraryExtensions = true, but there are shortcomings in the behavior, when the .css file is changed, rebuilding does not occur, because the watcher does not process changes in the .css file, it expects changes in the d.css.ts file, this file to file system, as you know, no.

I had to add all custom extensions to extraFileExtensions when call createWatchCompilerHost and turn off the option allowArbitraryExtensions = false I hope there will be a better solution in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants