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

Update js-beautify v1.15.1 to add new angular templating option #181

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

simon-knu
Copy link
Contributor

@simon-knu simon-knu commented Mar 15, 2024

This pull request addresses the open issue #177.

As I don't often submit pull requests, I would appreciate any feedback on the implementation and/or description.

@simon-knu simon-knu marked this pull request as draft March 15, 2024 10:38
@simon-knu
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Corellia SRL"

@simon-knu simon-knu marked this pull request as ready for review March 15, 2024 10:44
@simon-knu simon-knu changed the title Feature/angular templating Update js-beautify v1.15.1 to add new angular templating option Mar 15, 2024
@NesTeRDGIT
Copy link

NesTeRDGIT commented Mar 20, 2024

It seems to me that this PR will not work
Because:

  1. getTemplatingFormatOption always returns ['auto'] or ['none']. A should return parameters set by the user, for example ['angular'].
  2. HTMLFormatConfiguration.templating type should be - ('auto' | 'none' | 'angular' | 'django' | 'erb' | 'handlebars' | 'php' | 'smarty')[]

getTemplatingFormatOption function should return HTMLFormatConfiguration.templating
HTMLFormatConfiguration.templating itself must be filled in by the user in the VSCode parameters.
I don't know how to do this...

I think the VSCode repository needs to be updated: https://github.com/microsoft/vscode/blob/main/extensions/html-language-features/package.json

@simon-knu
Copy link
Contributor Author

@NesTeRDGIT You were right

I pushed a change, thank you for your feedback

IMO, the best thing to do is to keep the "boolean" property with the possibility to add the js-beautify configuration
Like that, we don't break existing configs

This means that we need to do something like that (see picture) in the vscode repository to be able to configure a boolean or an array of values

I just need to confirm that this kind of config is possible, but it would hope it is :)

image

What do you think ?

@NesTeRDGIT
Copy link

NesTeRDGIT commented Mar 21, 2024

I am not a VSCode developer
So I can't confirm this.
Here we need the opinion of VSCode developers

But I think VSCode will not accept such configuration because it is js-beautify private settings. These are not common options for all HTML extensions.

I was suggested to the js-beautify developers to develop an extension for VSCode, for example like: https://github.com/HookyQR/VSCodeBeautify
@bitwiseman

@bitwiseman
Copy link
Contributor

@NesTeRDGIT

I am not a VSCode developer So I can't confirm this. Here we need the opinion of VSCode developers

But I think VSCode will not accept such configuration because it is js-beautify private settings. These are not common options for all HTML extensions.

I was suggested to the js-beautify developers to develop an extension for VSCode, for example like: https://github.com/HookyQR/VSCodeBeautify @bitwiseman

The problem is that "js-beautify developers" is mostly me. I don't have the time to be primary maintainer on a VSCode plugin. If you are interested in forking the HookyQR beautifier plugin into https://github.com/beautifier and doing the work to spin up that project, please open an issue over in js-beautify and let's chat.

@aeschli
Copy link
Contributor

aeschli commented Mar 22, 2024

The HTML language service formatter and (VS Code) currently has an on/off setting for template support.
This mapped to auto or none.
This results in simple user experience and has worked well so far.

I wonder, do you know the reason why angular nor enabled by default in auto ?

@aeschli
Copy link
Contributor

aeschli commented Mar 22, 2024

Reading more about it, I see that the angular support got turned off due to performance reasons: beautifier/js-beautify#2246

The thing is, the HTML language server is intended for HTML as understood by browser. Templating languages are extensions of HTML and, if they extend the syntax, not HTML.
The HTML extension does it's best to not been thrown off by the extended syntax, and I know a lot of users still use it also for template languages, but the recommendation is to instead use an extension that is for that template language, e.g. an Angular extension

We can add the new options to vscode-html-languageservice, but I'm not in favor of complicating the settings of the HTML formatter in VS Code with a list of templating languages. That would give the impression that the HTML language extension is also for these languages, which it isn't.

@aeschli aeschli enabled auto-merge (squash) March 22, 2024 13:25
@aeschli aeschli merged commit a134f30 into microsoft:main Mar 22, 2024
3 of 4 checks passed
@simon-knu
Copy link
Contributor Author

simon-knu commented Mar 22, 2024

Thank you for the feedback @aeschli

I understand your point, but since the options are there, it feels a little bid odd (for me) to not allow users of VS Code to use it
I mean, with the configuration proposed here, it won't change anything for the "basic user"
When I say "basic user, I just mean user that doesn't want to configure more explicitly the formatting, don't get me wrong :)

That would give the impression that the HTML language extension is also for these languages, which it isn't.

It was maybe not intended at first, but with the dependency on js-beautify, now the HTML language extension is linked to the languages supported by the lib.

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.

5 participants