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

The index.html is minified and breaks some automation #17330

Closed
fooness opened this issue May 17, 2021 · 15 comments
Closed

The index.html is minified and breaks some automation #17330

fooness opened this issue May 17, 2021 · 15 comments
Labels
A-Packaging Packaging, signing, releasing P4 [OBSOLETE LABEL] Interesting — Not yet scheduled, will accept patches S-Tolerable Low/no impact on users T-Defect

Comments

@fooness
Copy link
Contributor

fooness commented May 17, 2021

The minified index.html (all content in one single line) in Element-Web v1.7.28 (and maybe one or two versions prior) completely breaks (our) Ansible-automated installation and configuration scripts.

Can this please be reverted? Element still offers no proper way of customisation and almost any new elements that are introduced use hardcoded color values instead of existing color variables … it’s simple not possible to properly modify element with just the color variable definitions in config.json, and now breaking the proper HTML-structure in index.html even makes it close to impossible to automatically change/add contents in that file, like e.g. adding custom css.

Please revert this to non-minified, proper HTML.

@turt2live turt2live changed the title the newly introduced minified index.html is a horrible idea that breaks automation … The index.html is minified and breaks some automation May 17, 2021
@turt2live
Copy link
Member

@fooness it would be great if this issue was written in a way where we were inclined to interact with it. Confrontational titles, unexplained asks for reverting a change, and assumption that this is intentional all make this undesirable to look at.

@t3chguy
Copy link
Member

t3chguy commented May 17, 2021

Please revert this to non-minified, proper HTML.

pretty sure it is quite proper & valid; https://validator.w3.org/nu/?doc=https%3A%2F%2Fapp.element.io%2F doesn't complain about the formatting of the file.

Can this please be reverted?

This was not an intentional change, but likely caused by a dependency being upgraded.

completely breaks (our) Ansible-automated installation and configuration scripts.

Your scripts were brittle then.

almost any new elements that are introduced use hardcoded color values instead of existing color variables

Please report these specifically because this should not be happening.

makes it close to impossible to automatically change/add contents in that file, like e.g. adding custom css.

Not impossible at all, you just have to write more resilient scripts that don't break when handed VALID HTML.

@jryans
Copy link
Collaborator

jryans commented May 17, 2021

The content of the index.html file is not a supported API surface, so it might change at any time.

This document minification was not done on purpose, but instead it happened as a side effect of upgrading. We would happily accept a PR to fix this, but it is also not a priority for the core team.

@jryans jryans added A-Packaging Packaging, signing, releasing P4 [OBSOLETE LABEL] Interesting — Not yet scheduled, will accept patches S-Tolerable Low/no impact on users labels May 17, 2021
@fooness
Copy link
Contributor Author

fooness commented May 17, 2021

@turt2live @t3chguy @jryans Pardon if this was a little bit harsh and direct. It’s just rather frustrating that established things in Element are oftentimes changed without detailed upgrade notes and oftentimes introduce some problems for people that want to make use of Element’s customisation features. Sorry for the strong words.

Can this please be reverted?

This was not an intentional change, but likely caused by a dependency being upgraded.

This makes me hope that we might get back the multi-line index.html file?

completely breaks (our) Ansible-automated installation and configuration scripts.

Your scripts were brittle then.

makes it close to impossible to automatically change/add contents in that file, like e.g. adding custom css.

Not impossible at all, you just have to write more resilient scripts that don't break when handed VALID HTML.

This is partly true; script that rely on e.g. replacing or adding lines kinda don’t work anymore when there’s no more than one single line in a file to work with.

This document minification was not done on purpose, but happened as a side effect of upgrading. We would happily accept a PR to fix this, but it is also not a priority for the core team.

We’re super happy to hear so. Is there any research/preparation work I could to for someone else to make the PR?

@fooness
Copy link
Contributor Author

fooness commented May 17, 2021

Maybe Switch back to release version of sanitize-html via #17231 has something to do with this?

@t3chguy
Copy link
Member

t3chguy commented May 17, 2021

Unlikely. We only call sanitize-html at runtime for validating user input.

@fooness
Copy link
Contributor Author

fooness commented May 17, 2021

Unlikely. We only call sanitize-html at runtime for validating user input.

Oh, okay sure … sanitizing user input indeed makes way more sense, than sanitizing code from the repo itself. Sorry.

Unfortunately, after looking through the last changelogs and pull request as well as searching for index.html in the element-web as well as matrix-react-sdk repo, I’m still not quite sure where this change could have been introduced.

PS: This did not happen in element-web v1.7.25 and v1.7.26

@t3chguy
Copy link
Member

t3chguy commented May 17, 2021

@fooness
Copy link
Contributor Author

fooness commented May 17, 2021

Likelu somewhere in https://github.com/vector-im/element-web/commits/develop/yarn.lock

The changes here look rather cryptic to me; lots of version/hash changes …

It seems that element-web v1.7.27-rc.1 introduced the problem; that release candidate was released on 2021-05-04 …

So the issue should be somwhere here, right? v1.7.26...v1.7.27-rc.1

@t3chguy
Copy link
Member

t3chguy commented May 17, 2021

@fooness
Copy link
Contributor Author

fooness commented May 17, 2021

v1.7.26...v1.7.27-rc.1diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519L114 looks relevant

Unfortunately the link does not work for me.

Screen Shot 2021-05-17 at 19 54 08

It seems that a package called html-minifier was replaced with html-minifier-terser via: 708f6a2#diff-51e4f558fae534656963876761c95b83b6ef5da5103c4adef6768219ed76c2deL5671-R5715

Screen Shot 2021-05-17 at 19 58 20

Could this be the culprit?

@t3chguy
Copy link
Member

t3chguy commented May 17, 2021

Sure, it could. That's a dependency of a dependency (of a dependency?)
You'd have to check the various dependency changelogs to see

@fooness
Copy link
Contributor Author

fooness commented May 18, 2021

@jryans @t3chguy Might I please ask you to have a look at the pull request in #17349? Unfortunately, I’m not sure how to properly run tests and whatever else is needed to verify that everything works as intended, but I don’t think there should be any problems with this change?

@fooness
Copy link
Contributor Author

fooness commented May 18, 2021

Fixed in #17349 and therefore closed.

@fooness fooness closed this as completed May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Packaging Packaging, signing, releasing P4 [OBSOLETE LABEL] Interesting — Not yet scheduled, will accept patches S-Tolerable Low/no impact on users T-Defect
Projects
None yet
Development

No branches or pull requests

4 participants