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

Remove Need for File Extensions in TypeScript Language Definitions #26413

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented May 10, 2017

Part of #25740

To support TS Server plugins for languages like angular, we will allow extensions to register new langauges for TypeScript to watch. The angular language for example would want ng-html files to also be uploaded to TypeScript for checking

The current language definitions all define both a set of language modes they support and a set of file extensions. The file extension part is unnessiary and may be incorrect depending on how a user sets up their file.associations in the workspace.

This change removes the extensions part so that we always make use of the language mode

Part of microsoft#25740

To support TS Server plugins for languages like angular, we will allow extensions to register new langauges for TypeScript to watch. The angular language for example would want ng-html files to also be uploaded to TypeScript for checking

The current language definitions all define both a set of language modes they support and a set of file extensions. The file extension part is unnessiary and may be incorrect depending on how a user sets up their `file.associations` in the workspace. This change removes the extensions part so that we always make use of the language mode
@mjbvz mjbvz self-assigned this May 10, 2017
@mjbvz mjbvz requested a review from dbaeumer May 10, 2017 21:07
@dbaeumer
Copy link
Member

dbaeumer commented May 11, 2017

@mjbvz actually I know that there was a reason why I added this but I can't remember why. I will ping if I can recall it. Sorry for not being more helpful.

The change looks good to me coding wise.

@mjbvz mjbvz added this to the May 2017 milestone May 11, 2017
@mjbvz mjbvz merged commit 0d5c9f4 into microsoft:master May 11, 2017
@mjbvz
Copy link
Collaborator Author

mjbvz commented May 11, 2017

Thanks for taking a look @dbaeumer. My main concern with this change was performance since we now will be calling openTextDocument more often. I tested various js+ts scenarios though and didn't notice any regressions. Let me know if you remember the rational behind using the file extensions

@dbaeumer
Copy link
Member

@mjbvz now a bell rings. openTextDocument might be bad :-) The reason is that openTextDocument sends and open event (if not open already) which sends the document over to the server. If the document is already open that this is a no op, but if not we might put pressure on the server for no benefit. Assume the server reports errors on file not open (which might come at some point with the builder) then we would open every file in the server that has errors although it is not visible in VS Code.

mjbvz added a commit to mjbvz/vscode that referenced this pull request Jun 20, 2017
**Bug**
Typescript previously used file extensions to determine which files it handles or not. This was not great because it duplicates information and also fails to account for `file.associations`. For the upcoming work on TS server plugin work, I switched to use openTextDocument for this check instead. However Dirk pointed out that this brings its own problems: microsoft#26413 (comment) . I don't see any ways around this using the current api, and I also don't see how we can fix the `file.associations` issue using the old extensions based approach

**Fix**
Add a new `getLanguage` api to VSCode. This api returns the language normally associated with a resource (uri). It does not take into account any language mode that a user has explicitly changed or the first line based language detection (since the goal was to avoid opening the resource in the first place).

It's not clear to me if the current implementation based on openTextDocument will actually cause any problems. This new API may help us avoid some perf issues but also is not perfect. Let me know if you have any thoughts on this
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants