[fix] static svelte:element not replaced with tag in production mode

[ramonsnir]

This aims to fix #7937 by adding a new condition for opting out of the .innerHTML optimization in production mode.

The change is based on the changes from #7390.

Before this change, Svelte 3.51.0 compiles the parent of <svelte:element this="slot" name="empty" /> with div.innerHTML = '<svelte:element name="empty"></svelte:element>';, which is not the intended result. The final HTML should be <slot name="empty" />.

[tanhauhau]

it would be great if this can generate

div.innerHTML = '<p name="empty"></p>'

since "p" is static

[ramonsnir]

@tanhauhau Keeping as two separate commits for the duration of the discussion. It now compiles to that, but I'm slightly worried that it would cause other regressions :slightly_smiling_face: npm run test:unit doesn't detect other regressions but I'm not familiar enough with the coverage.

[baseballyama]

@tanhauhau

it would be great if this can generate

Already once I implemented this optimization but removed. Same reason as https://github.com/sveltejs/svelte/pull/7942#pullrequestreview-1142286372 .

[ramonsnir]

Already once I implemented this optimization but removed. Same reason as #7942 (review) .

For other readers' context, the comment linked (which @baseballyama wrote) says:

IMO, it would be better to raise a development-time warning compared to optimize it by a compiler.

Note that this PR's objective isn't to add an optimization, but to fix a current bug in the Svetle compiler: It fails to allow for a way to emit a static HTML <slot />, as verbatim usage is taken to refer to Svelte slots and svelte:element has this bug. @tanhauhau's suggestion isn't to add a new optimization, but to keep an existing optimization as-is and fix the way it handles svelte:element.

I see the two possible fixes (see each commit in this PR for each option):

1. What I originally did: Disabled the optimization for the case of svelte:element. It could be nice to add a dev warning for such cases to point out to the user that if they were to use the HTML tag directly instead of a static svelte:element, the compiler could be more efficient. That warning would have to exclude tag names that the Svelte compiler would refuse to emit if they were rewritten without svelte:element, specifically slot (and sometimes custom elements also misbehave with Svelte and/or TypeScript).
2. What @tanhauhau suggested: Fix the bug in the existing optimization. At this point, there's no need to show a dev warning because both ways to write the tag have exactly the same output. It might be desirable to add an eslint rule for this, but I believe that eslint rules for Svelte are in a different repository, and it isn't a very important eslint rule to add.

[baseballyama]

In any case, I think this point can be handled in another PR.

At this point, there's no need to show a dev warning because both ways to write the tag have exactly the same output.

Still I think we should point out to our users to write more sophisticated code. If there is 1 dirty code, whole codebase will become dirty due to the Broken windows theory.

[ramonsnir]

Still I think we should point out to our users to write more sophisticated code. If there is 1 dirty code, whole codebase will become dirty due to the Broken windows theory.

~~I agree with that a lot, which is why I run rust's clippy with all categories up to pedantic enabled and maintain a strict eslint configuration for TypeScript and Svelte projects - but styling is for linters.~~

Retracted.

[baseballyama]

We discussed with maintainers and concution is that we should not implement warnings because compiler can not decide that reactive statement is essentially unnecessary or not.

e.g.

<script>
  export const foo = 1;
  export const bar = 1;
  export const baz = 1;

  // For now, all dependencies are static, but maybe in the future, one or more variable will be dynamic.
  // So Intentionally, I used `$:` for here for preventing to make a bug.
  $: qux = foo + bar + baz;
</script>

[ramonsnir]

@baseballyama Wrong PR? That comment looks related to #7942

[baseballyama]

@baseballyama Wrong PR? That comment looks related to #7942

Sorry! I copied the comment to #7942

[baseballyama]

it would be great if this can generate

div.innerHTML = '<p name="empty"></p>'

since "p" is static

I implemented this optimization and I fixed the above bug. Please check this and merge this if LGTM. @ramonsnir

https://github.com/ramonsnir/svelte/pull/1

[ramonsnir]

I implemented this optimization and I fixed the above bug. Please check this and merge this if LGTM. @ramonsnir

ramonsnir#1

Merged. Let me know if you'd like me to squash the commits.