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

Expose ts.matchFiles as public API to make implementing readDirectory easier. #13793

Open
krisselden opened this issue Jan 31, 2017 · 10 comments
Labels
API Relates to the public API for TypeScript Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@krisselden
Copy link

Problem:

When implementing a custom host that virtualizes the file system, the readDirectory method is cumbersome to implement in a way that matches tsc behavior.

If you look at the hosts within typescript, the matchFiles method was added to unify behavior with readDirectory between hosts.

While matchFiles is exported, it isn't in the type definition files so I'm assuming it is "private" API.

Either the host should be able to provide a getFileSystemEntries(path: string) => { files: string[], directories: string[] } to get a readDirectory implementation or expose matchFiles.

I'm working on improving the broccoli-typescript-compiler broccoli plugin to have more parity with tsc and VS Code, but I need to virtualize the FS, since broccoli builds from tmp directory and I want the node_modules/@types discovery, tsconfig extends, etc all to work, I need to pretend the tmp dir is inside the project.

Thanks

@mhegazy
Copy link
Contributor

mhegazy commented Jan 31, 2017

Our policy has been to only expose APIs that represent language constructs/concepts; helpers and utilities are kept private to limit the API surface area.

This specific API falls under the utilities category, and it kinda falls between two abstraction levels (System, and CompilerHost). We have generally responded to similar requests by asking about duplicating the code, specially for small helpers, would that be an option?

If understand correctly, you are implementing System, and not not using the existing ts.sys, if so, can you use ts.sys?

@DanielRosenwasser DanielRosenwasser added the API Relates to the public API for TypeScript label Feb 1, 2017
@krisselden
Copy link
Author

@mhegazy you cannot make a host that reasonably supports globbing that virtualizes the file system, look at your own examples of hosts, ts.sys doesn't work here because it doesn't allow the file system to be virtualized.

The globing is somewhat unique to TypeScript, it doesn't quite align with other node glob libraries so if you want to ensure your host behaves similar to tsc would require using private API or copying and pasting a large amount of code.

I need to virtualize the file system to fit into how broccoli build system works in tmp dirs, but give errors and resolve config and node_modules from the project dir.

@krisselden
Copy link
Author

@mhegazy I generally understand the policy regarding API surface but there is a massive amount of useful things in this codebase that is private API, stuff that would make for some amazing static analysis like the flow, container, bindings, imports, exports. Also, non of the code generation stuff is exposed, no plugins etc. Exposing this stuff could make for some wickedly cool satellite projects. I understand not wanting to back yourself into a corner with the API but sometimes worse is better.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2017

The code is open source, and has always been, so to be clear we do not intend to hide anything.

The main issue is that more API surface area means more promises to not break API shape and semantics, and we really hate to break ppl. What complicates matters is we have certain use cases, for APIs, these are the things we test for, other use cases could be broken with no notification.

For the emitter, we have been working on exposing these, see #13765, #13764 and #13763. The printer API is already in, see #13761, and the factory is in review, see #13825.

For other APIs, please let us know what APIs you want to use, and how you intend to use them, and we would be happy to expose them. we just need to make sure we understand how users are using the API to make sure we know what changes we can and what we can not make in the future.

As for the original issue.. if you really, really, really need it, I suppose we can expose it.

@dsherret
Copy link
Contributor

dsherret commented Jul 2, 2017

+1, I am working on trying to find all the source files associated with a tsconfig.json and using the existing sys.readDirectory isn't an option because it doesn't work in a virtualized file system.

It would be nice to reuse the code in ts.matchFiles.

@krisselden
Copy link
Author

For now I am just relying on the fact it is exported and hoping it won't break but I consider readDirectory to be a poor contract for a host to be able to provide and hope to ensure the include/exclude function the way people expect.

@mhegazy mhegazy added the Help Wanted You can do this label Nov 9, 2017
@mhegazy mhegazy added this to the Community milestone Nov 9, 2017
dsherret added a commit to dsherret/TypeScript that referenced this issue Jan 5, 2018
@dsherret dsherret mentioned this issue Jan 5, 2018
dsherret added a commit to dsherret/TypeScript that referenced this issue Jan 5, 2018
dsherret added a commit to dsherret/TypeScript that referenced this issue Jan 5, 2018
@AviVahl
Copy link

AviVahl commented Sep 30, 2018

@mhegazy can this be looked at again? I've encountered the exact same need.

I have my own implementation for an in-memory file system which I would like to use for tests and caching. Implementing a ParseConfigHost or LanguageServiceHost is easy up to the point where you have to provide a custom readDirectory. I order to get the exact same behavior of TypeScript, I shamelessly augmented the types:

import * as ts from 'typescript'
declare module 'typescript' {
    function matchFiles(
        path: string, extensions: ReadonlyArray<string>, excludes: ReadonlyArray<string>,
        includes: ReadonlyArray<string>, useCaseSensitiveFileNames: boolean, currentDirectory: string,
        depth: number | undefined, getFileSystemEntries: (path: string) => FileSystemEntries): string[]

    export interface FileSystemEntries {
        files: ReadonlyArray<string>
        directories: ReadonlyArray<string>
    }

    export interface JsonSourceFile extends ts.SourceFile {
        parseDiagnostics: ts.Diagnostic[]
    }
    export function getNewLineCharacter(options: ts.CompilerOptions | ts.PrinterOptions): string
}

This can obviously break at any given future TS release without me knowing at build time.

Also, even with matchFiles() exposed, I had to implement:

    function getFileSystemEntries(path: string): ts.FileSystemEntries {
        const files: string[] = []
        const directories: string[] = []
        const entries = syncFs.listDirectorySync(path)
        for (const entry of entries) {
            const node = syncFs.getStatsSync(syncFs.path.join(path, entry))
            if (node.isFile()) {
                files.push(entry)
            } else if (node.isDirectory()) {
                directories.push(entry)
            }
        }
        return {files, directories}
    }

The parseDiagnostics field and getNewLineCharacter() helper are two additional symbols I found useful when creating a custom language service.

@cspotcode
Copy link

@AviVahl I'm trying to do the same thing and just encountered readDirectory myself. My goal is to publish an in-memory implementation of ts.System for use in browsers demos, experiments, etc. Do you have a link to what you're working on?

@usergenic
Copy link

I'm currently having to implement a readDirectory etc and I was really hoping this had been addressed 3 years ago when brought up.

devversion added a commit to devversion/material2 that referenced this issue Jul 9, 2020
In some situations, developers will run `ng update` from a sub directory
of the project. This currently results in a noop migration as file
system paths were computed incorrectly. This is because ng-update
always assumed that the CWD is the workspace root in the real file
system. This is not the case and therefore threw off path resolution.

We can fix this by determining the workspace file system path from
the virtual file system host tree's. This is not ideal, but best
solution for now until we can parse/resolve TypeScript projects
purely from within the virtual file system. This is currently
not easy to implement and requires helpers from TS which are
not exposed yet. See:
microsoft/TypeScript#13793.

This is tracked with: COMP-387.

Fixes angular#19779.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Relates to the public API for TypeScript Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

8 participants