Skip to content

Remove support for MINIFY_HTML #22297

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

Closed
wants to merge 1 commit into from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jul 31, 2024

This setting has been causing some problems (see #22188) so I think we should just remove it.

My understanding is that most users of emscripten use it to generate JS and not html. Going forward I'm tempted to also remove the --shell-file command line flag since I'm not sure there any many compelling use cases for a custom shell html. While html generation is certainly useful for quick testing and examples, its not really something web sites are likely to use in production since they will likely have their own existing flow for build html.

Fixes: #22188

@sbc100 sbc100 requested a review from kripken July 31, 2024 12:45
@sbc100 sbc100 force-pushed the remove_minify_html branch from d1cbf56 to 3ad9bb5 Compare July 31, 2024 12:45
@sbc100 sbc100 requested a review from dschuff July 31, 2024 12:45
@sbc100 sbc100 force-pushed the remove_minify_html branch 3 times, most recently from 4697791 to fd582fb Compare July 31, 2024 13:57
@kripken
Copy link
Member

kripken commented Jul 31, 2024

If the problem in #22188 is unfixable then I agree, we should remove this feature as it is broken. But if it is fixable then I think we'd just be pushing the problem forward to users that want this minification? In that case I think we should try to fix it here once in upstream.

Or, to put it another way, I think HTML minification is reasonable for us to do, the same as JS minification. It's a good out of the box experience. But if there aren't tools that easily work for us, then I agree we should give up on it.

@Akaricchi
Copy link
Contributor

I still use a hacky custom shell to deploy the web builds of Taisei, though I'm not too happy about it. I'd like to properly modularize it eventually, but I have other priorities (as a very much not-a-web-dev, I find every excuse to not touch that stuff).

I've also encountered #22188, and my "solution" was to replace the mis-minified spaces with   tokens.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 6, 2024

If the problem in #22188 is unfixable then I agree, we should remove this feature as it is broken. But if it is fixable then I think we'd just be pushing the problem forward to users that want this minification? In that case I think we should try to fix it here once in upstream.

I think fixing it would involved digging deep into how terser html minifiction works and understanding all the edge cases. Not something I have much of an appetite for, especially if its not a feature anyone actually needed/uses.

Or, to put it another way, I think HTML minification is reasonable for us to do, the same as JS minification. It's a good out of the box experience. But if there aren't tools that easily work for us, then I agree we should give up on it.

I disagree. Almost all of our users rely on out generated JS and depend on it being small. Almost none of our user, I imagine, are depending on emscripten to build their html file. Why would they? This feature is great so small demos and hello world examples and testing, but I don't see any use of it in production. In production surely you want to author your own HTML from scratch.

@kripken
Copy link
Member

kripken commented Aug 6, 2024

@sbc100 sounds reasonable, but I'm not actually sure we know how many users use our HTML generation and optimization. I assume @juj does, but I could be wrong?

Might also be worth asking on the mailing list. If we can't find a significant amount of users then I am ok with removing this.

@sbc100 sbc100 force-pushed the remove_minify_html branch from fd582fb to e348df9 Compare August 8, 2024 18:17
This setting has been causing some problems (see emscripten-core#22188) so I think
we should just remove it.

My understanding is that most users of emscripten use it to generate
JS and not html.  Going forward I'm tempted to also remove the
`--shell-file` command line flag since I'm not sure there any many
compelling use cases for a custom shell html.  While html generation
is certainly useful for quick testing and examples, its not really
something web sites are likely to use in production since they will
likely have their own existing flow for build html.

Fixes: emscripten-core#22188
@sbc100 sbc100 force-pushed the remove_minify_html branch from e348df9 to c7052a6 Compare August 8, 2024 19:50
@juj
Copy link
Collaborator

juj commented Aug 9, 2024

This setting has been causing some problems (see #22188) so I think we should just remove it.

I'm not sure if this is the best approach. Why drop useful features just because a user runs into a bug?

I use this html-minifier based approach in my engine builds, and it presents a full solution from CMake to final output, especially for SINGLE_FILE cases. It is very convenient.

Just because there are many users that are building to .js does not mean that building to .html with minification could not be useful to other sets of users?

I've posted a PR #22346 to fix the root issue in the minification, and an upstream ticket terser/html-minifier-terser#179 to convey the unexpected behavior to the developers of the minifier.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 9, 2024

Thanks for the info @juj and for filing the bug.

Can I ask about "I use this html-minifier based approach in my engine builds, and it presents a full solution from CMake to final output, "? Are you using this feature in combination with --shell-file to produce production builds? Or are you using the emscripten-default html for debugging/testing?

@Akaricchi
Copy link
Contributor

Are you using this feature in combination with --shell-file to produce production builds?

Not the person you're asking, but I am currently using it like that.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 9, 2024

Are you using this feature in combination with --shell-file to produce production builds?

Not the person you're asking, but I am currently using it like that.

Is there some reason you using emscripten to produce the html file? I'm trying to understand the advantage to using this feature. Why not just ship the html that you have already written?

@Akaricchi
Copy link
Contributor

Is there some reason you using emscripten to produce the html file? I'm trying to understand the advantage to using this feature. Why not just ship the html that you have already written?

I suppose I could do that. It's just nice to have html minification without getting any web-specific tools involved, as @juj said. It won't be a huge problem for me if this is removed, but it sure is annoying having to unbreak my build system every time I update Emscripten. I build for a variety of platforms and this toolchain is the only one that has this problem.

@juj
Copy link
Collaborator

juj commented Aug 20, 2024

Are you using this feature in combination with --shell-file to produce production builds?

Yes I am. For examples, Bat Chase and Spider Solitaire both build to HTML. First one uses default minimal runtime shell, the second one uses a custom --shell-file directive for minimal runtime.

@juj
Copy link
Collaborator

juj commented Aug 20, 2024

Is there some reason you using emscripten to produce the html file? I'm trying to understand the advantage to using this feature.

This allows one to integrate the generation of the page via the build system.

Why not just ship the html that you have already written?

I don't have a separate html page. I write the shell html file to be the page.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 20, 2024

Fair enough. I was kind of hoping that folks could use external tools or build systems to produce the html they need. It seems like something that a separate tool could do, but I sounds like MINIMAL_RUNTIME in particular has more deep integration with the HTML output.

As usual, I'm trying to reduce the number of things emcc itself does to try to bring down the complexity and testing surface. I'll abandon this effort for not.

@sbc100 sbc100 closed this Aug 20, 2024
@sbc100 sbc100 deleted the remove_minify_html branch January 3, 2025 22:45
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.

In release mode, the "minification" of shell.html remove spaces that should not be removed
4 participants