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

refactor: simplify hosts to directly assign tsModule.sys where possible #349

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jun 6, 2022

Summary

Simplify host files with public func = tsModules.sys.func where possible

  • Technically a follow-up to test: add initial unit test suite #321 as I was trying to get this to work at the same time, but ran into some testing issues (see below) and then backlogged it for a while as such

Details

  • no need to duplicate types this way, which can and have changed over time (some of the function args have more parameters etc now actually) -- it's always the same typings this way

  • also reorganize host.ts to have similar categories of functions near each other, instead of a mix of functions wherever

    • similar to how I organized the tests for host.ts as well
  • shrink the code a bit this way too

  • add a comment about getDefaultLibFileName's confusing naming pointing to the TS issues about how this is an old mistake but changing it now would be breaking (LanguageServiceHost.getDefaultLibFileName is confusing microsoft/TypeScript#35318)

  • this is also how the TS Wiki recommends setting up hosts

    • NOTE: because of how tsproxy works to support alternate TS implementations, this does require that tsModule exists at the time of instantiation, i.e. that setTypescriptModule has already been called
      • for host.ts, LanguageServiceHost is only instantiated after setTypescriptModule, but for diagnostics-format-host.ts, it is immediately instantiated (at the bottom of the file), hence why getCurrentDirectory can't just be assigned to tsModule.sys
        • there is a way to fix this, but the refactoring for that is more complex as it would require creating in index.ts and then passing it as an argument -- would want to refactor more at that point too, so leaving that out for now in this otherwise small, isolated refactor
    • for a different, but related reason, the host.trace tests have to mock console instead of just console.log, since trace would just be set to the old, unmocked console.log otherwise
      • as it's assigned directly to console.log now

@agilgur5 agilgur5 added the kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs label Jun 6, 2022
- no need to duplicate types this way, which can and have changed over
  time -- it's always the same typings this way

- also reorganize `host.ts` to have similar categories of functions near
  each other, instead of a mix of functions wherever
  - similar to how I organized the tests for `host` as well
- shrink the code a bit this way too

- add a comment about `getDefaultLibFileName`'s confusing naming
  pointing to the TS issues about how this is an old mistake but
  changing it now would be breaking

- this is also how the TS Wiki recommends setting up hosts: https://github.com/microsoft/TypeScript/wiki/Using-the-Compiler-API#incremental-build-support-using-the-language-services
  - NOTE: because of how `tsproxy` works to support alternate TS
    implementations, this does require that `tsModule` _exists_ at the
    time of instantiation, i.e. that `setTypescriptModule` has already
    been called
    - for `host.ts`, `LanguageServiceHost` is only instantiated after
      `setTypescriptModule`, but for `diagnostics-format-host.ts`, it is
      immediately instantiated (at the bottom of the file), hence why
      `getCurrentDirectory` can't just be assigned to `tsModule.sys`
      - there is a way to fix this, but the refactoring is more complex
        as it would require creating in `index.ts` and then passing it
        as an argument -- would want to refactor more at that point too,
        so leaving that out for now in this otherwise small, isolated
        refactor
  - for a different, but related reason, the `host.trace` tests have to
    mock `console` instead of just `console.log`, since `trace` would
    just be set to the old, unmocked `console.log` otherwise
    - as it's assigned directly to `console.log` now
@ezolenko ezolenko merged commit d32cf83 into ezolenko:master Jun 7, 2022
@agilgur5 agilgur5 deleted the refactor-simplify-hosts branch July 2, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants