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

Add option to disable custom elements inlining #2115

Merged

Conversation

cexbrayat
Copy link
Contributor

Description

  • Source branch in your fork has meaningful name (not main)

Fixes Issue:
#2113

Before Merge Checklist

These items can be completed after PR is created.

(Check any items that are not applicable (NA) for this PR)

  • JavaScript implementation
  • Python implementation (NA if HTML beautifier)
  • Added Tests to data file(s)
  • Added command-line option(s) (NA if
  • README.md documents new feature/option(s)

@cexbrayat cexbrayat force-pushed the feat/2113-inline-custom-elements-behavior branch from 5c7eb7e to dd687ae Compare November 11, 2022 11:48
@sagecoretech
Copy link

sagecoretech commented Jan 27, 2023

Hello,

This was something i needed and have code ready to commit. I added option inline_custom_tags.

What is the best way to contribute these 2 lined of code?

NEVERMIND - my code is identical to this commit. Thanks!

@bitwiseman
Copy link
Member

bitwiseman commented Jun 30, 2023

@cexbrayat @sagecoretech @Ericlm
One of the most common problems encountered in this library is a change to the default behavior that breaks other user scenarios not currently covered by tests. I'm sorry.

Generally, f a change to the default behavior is definably more correct, which is to say we can clearly state the old behavior is wrong and the new behavior at least "more right" we will keep the new behavior. After quite a bit of searching, it does look like #2101 which makes all custom elements inline elements is more right, but can be wrong just as often.

Adding a special option to address this foible is going result in beautifier behavior be wrong in a different way just as often as it is right.

Rather than adding a new option like you did so far, would you be willing to implement some form of #1668 instead? As part of that change making the inline list regex match instead of exact match? If you do that, you could remove the special code for elements containing a dash and leave it to people to configure the inline behavior as they see fit.

If you can't do that then we should probably revert #2101 that introduced this problem (sorry @lgarron !). If you choose to go this route, please add tests with comments that describe your use case so at future attempts to change this will be required to address them.

But I really hope you will consider doing #1668. It is more work, but #1668 would have broad benefits for the many users of this project.

@lgarron
Copy link
Contributor

lgarron commented Jul 1, 2023

Do I understand correctly that the issue "js-beautify changed its default behaviour" rather than "the new default behaviour is causing incorrect output"? I don't completely understand some of the linked issues, but it sounds like they can be solved by hardcoding certain lists of elements to the HTML spec.

I'm only one person, but from my perspective this is the situation:

I want to be able to trust my tools to have reliably correct behaviour, and I want to promote the same for everyone. Custom elements are a core part of modern web development, and HTML/CSS development is already subtle enough without potential breaking changes from unexpected sources. It took me a while to track down what was even happening to my code, and I have well over a decade of experience with these DOM parsing quirks.

If #2101 is reverted, I will strongly advocate for VS Code to avoid breaking code in its default formatter, whether that means running js-beautify with potential custom options, or switching to another formatter that preserves layout.

@bitwiseman
Copy link
Member

@lgarron

If #2101 is reverted, I will strongly advocate for VS Code to avoid breaking code in its default formatter, whether that means running js-beautify with potential custom options, or switching to another formatter that preserves layout.

Are you intending to communicate some sort of threat here?

Is anyone on this PR willing to do at least what I asked above:

make the inline list a regex match instead of exact match? If you do that, we could remove the special code for elements containing a dash and leave it to people to configure the inline behavior as they see fit.

@cexbrayat
Copy link
Contributor Author

@bitwiseman Thanks for taking a look at the PR. I personally don't have the bandwidth to dig into what you suggested, so a revert is fine by me if this PR is not enough 👍

@bitwiseman
Copy link
Member

Blarg. I don't care anymore.

@bitwiseman bitwiseman merged commit bcc4213 into beautifier:main Jul 20, 2023
12 checks passed
@cexbrayat
Copy link
Contributor Author

@bitwiseman Thanks for taking a look and merging it!

I may have missed something, as the option is not available in the TS types. See this small repro on the TypeScript Playground: https://www.typescriptlang.org/play?ssl=6&ssc=4&pln=5&pc=4#code/JYWwDg9gTgLgBAIwKYEMCuNgDMCectQQhwDkAVgM4C0y6muJA3AFDO0bY4B0AFjCABsAFCQA8AE2AA3AHwAJJAIERRAekmySAGjgBvZnENxgAO3FITMAPoVgALyQAuOACYtBo6YGmkVgMZoFDBEVopIIBYwFM5YKAIUSMwAvgCUjEA

I can open a PR to fix this if you let me know where to look.

@cexbrayat cexbrayat deleted the feat/2113-inline-custom-elements-behavior branch July 20, 2023 08:46
@cexbrayat
Copy link
Contributor Author

Ho my bad, the types come from @types/js-beautify, so I'll open a PR there.

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.

None yet

4 participants