Skip to content

Dont use baseURL relative absolute paths in declaration emit, use absolute paths in bundle emit #26341

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

Merged

Conversation

weswigham
Copy link
Member

Fixes #25685

@weswigham weswigham requested review from RyanCavanaugh and a user August 9, 2018 20:56
@@ -5334,6 +5334,7 @@ namespace ts {
fileExists?(path: string): boolean;
readFile?(path: string): string | undefined;
getSourceFiles?(): ReadonlyArray<SourceFile>; // Used for cached resolutions to find symlinks without traversing the fs (again)
Copy link

Choose a reason for hiding this comment

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

I think the API would be better if this and getCommonSourceDirectory weren't optional members of this interface, and were just passed in as values in the one place they're needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmmm they're on the interface because that interface is how the declaration emitter shuffles bits of its emit host into the checker to fulfill these needs. And I don't particularly want to pass in a second host object with the kinda-nice-to-have-for-one-of-the-entry-points methods (especially since {moduleResolverHost: host, declarationModuleResolverHost: host} in declarations.ts would look like we were going mad).

Copy link

Choose a reason for hiding this comment

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

I didn't mean to add a second host for those methods, but that a host isn't necessary at all. Both methods are immediately called in the first function and not used by anything else in moduleSpecifiers.
Making the method optional seems to cover up problems, such as the fact that getCommonSourceDirectory isn't defined on TypeCheckerHost (although it might happen to be defined at runtime anyway, though possibly with a different type), and users of other functions will think they need to provide these methods even when they're unused outside of getModuleSpecifierForDeclarationFile. It also looks like host.getSourceFiles() is non-optional so testing for its existence is dead code.

Copy link
Member Author

Choose a reason for hiding this comment

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

A host is merely a group of argument methods to delegate to, no? If I need two, it's reasonable to group them into a second host object.

@weswigham weswigham merged commit 19e04b2 into microsoft:master Aug 10, 2018
@weswigham weswigham deleted the prefer-relative-declaration-paths branch August 10, 2018 19:54
errendir added a commit to errendir/TypeScript that referenced this pull request Aug 11, 2018
* origin/master: (283 commits)
  Don't error on destructure of private property with computed property syntax (microsoft#26360)
  getDefaultExportInfo: Use `getImmediateAliasedSymbol` instead of `getAliasedSymbol` (microsoft#26364)
  review comments
  restore old algorithm
  Dont use baseURL relative absolute paths in declaration emit, use absolute paths in bundle emit (microsoft#26341)
  Update user baselines (microsoft#26358)
  Don't store @template constraint in a TypeParameterDeclaration node (microsoft#26283)
  fixAddMissingMember: Support interface and don't crash on type parameter (microsoft#25995)
  Don't include class getter in spread type (microsoft#26287)
  Don't crash on computed property in destructure (microsoft#26334)
  Check the ambientness of a symbol name before attempting to trim it (microsoft#26312)
  Still generate signatures in SkipContextSensitive mode just to match on return types (microsoft#25937)
  fix handling if there is no commonPrefix
  Actually add sorting of elaboration text to user baselines
  Ping ryan instead of mohammed for user PRs now
  handle failed lookups
  make it work for root directory
  really, really fix test(?)
  add test
  fix commonPrefix handling
  ...
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.

1 participant