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

fix(sfc-playground): remove comment nodes in the production env #9594

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Alfred-Skyblue
Copy link
Member

@Alfred-Skyblue Alfred-Skyblue commented Nov 13, 2023

fixed #9591

In a production env, comments should be removed.

@Alfred-Skyblue Alfred-Skyblue force-pushed the fix/sfc-playground/prod-comment branch from 6cf7f2f to 42a7069 Compare November 13, 2023 12:36
Copy link

github-actions bot commented Nov 13, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 90.8 kB 34.5 kB 31.1 kB
vue.global.prod.js 148 kB 53.7 kB 48 kB

Usages

Name Size Gzip Brotli
createApp 50.8 kB 19.9 kB 18.1 kB
createSSRApp 54.1 kB 21.2 kB 19.3 kB
defineCustomElement 53.1 kB 20.6 kB 18.8 kB
overall 64.6 kB 24.9 kB 22.6 kB

@Alfred-Skyblue Alfred-Skyblue force-pushed the fix/sfc-playground/prod-comment branch from 42a7069 to 3b6d1c6 Compare November 13, 2023 12:39
@pikax pikax added the 🧹 p1-chore Priority 1: this doesn't change code behavior. label Nov 22, 2023
@skirtles-code
Copy link
Contributor

I'm not sure about this.

By default, production builds do remove comments, but it's configurable. It isn't safe to assume that comments are always stripped from production builds.

While I can see the argument for making the Playground consistent with the default build configuration, I think it makes it much less useful for bug reproductions. The compiler code for handling comments: false is very simple and unlikely to be required in bug reproductions. On the other hand, comments: true leads to a lot of interesting runtime edge cases involving attribute inheritance, Transitions, KeepAlive, etc.. It's easy to forget that these edge cases can also apply to production builds, so having the Playground default to comments: true (as it does currently) is a useful tool for catching those cases.

Even if this PR is merged, I don't think it is a fix for the original issue, it just hides it in the Playground. The original problem still exists in a Vite-based project with comments: true configured.

@haoqunjiang haoqunjiang added scope: playground need discussion need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. and removed need discussion labels May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 p1-chore Priority 1: this doesn't change code behavior. need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. scope: playground
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

Attribute Inheritance broken when comment in template root
5 participants