Skip to content

Add support for extraFileExtensions on WatchCompilerHost #37726

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
merged 1 commit into from
Apr 1, 2020

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat merged commit 0b38a9a into master Apr 1, 2020
@sheetalkamat sheetalkamat deleted the extraFileExtensions branch April 1, 2020 18:13
@DanielRosenwasser
Copy link
Member

So just to be clear, there's a break in the API which needs to be documented - but any consumer that updates its usages of this function should be fixed?

@DanielRosenwasser
Copy link
Member

If so, can somebody link me to the thing that caused the original break?

@uniqueiniquity
Copy link
Contributor

#37278 removed the function that was being used. I believe it was internal.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 1, 2020

In that thread @JoshuaKGoldberg removed something that appeared unused. If removing it affected things, then I assume it really was used. Can someone explain what happened step-by-step?

@uniqueiniquity
Copy link
Contributor

From my perspective, Brad tagged me in typescript-eslint/typescript-eslint#1813, I looped in Sheetal, and she quickly identified the issue and put up a fix.

@sheetalkamat Could you give us a sense of what happened regarding Josh's PR?

@sheetalkamat
Copy link
Member Author

@DanielRosenwasser There was onCachedDirectoryStructureHostCreate method on WatchCompilerHost which was internal (intentionally) that we never re;lied on and hence was removed (the unused variable host was set in that method callback)` Given this was internal method, no body shouldnt have been relying on that but that was not the case and hence the break for eslint. I believe PR #37278 is correct though in removing those things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants