glimmerjs / glimmer-vm

MIT License
1.13k stars 190 forks source link

SVG element parsing regression for svg element tag names with camelCase: `ReferenceError: radialGradient is not defined` when converting to gts-component #1550

Closed johanrd closed 4 months ago

johanrd commented 7 months ago

I get a regression when converting a template-only-hbs-component into a template-only-gts-component.

The gts-parser seems to think that the svg element radialGradient is a glimmer component, and not an svg element:

Error in browser console:

radial-gradient-reproduction.ts:1 Uncaught (in promise) ReferenceError: radialGradient is not defined
  at Object.scope (radial-gradient.ts:1:956)
  at meta (opcode-compiler.js:632:1)

Reproduction:

// radial-gradient-reproduction.gts:

const RadialGradientReproduction = <template>
  <svg viewBox="0 0 10 10" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <defs>
      <radialGradient id="myGradient">
        <stop offset="10%" stop-color="gold" />
        <stop offset="95%" stop-color="red" />
      </radialGradient>
    </defs>
    <circle cx="5" cy="5" r="4" fill="url('#myGradient')" />
  </svg>
</template>;

export default RadialGradientReproduction;

Environment:

"@glimmer/component": "^1.1.2",
"@embroider/compat": "3.4.3",
"@embroider/core": "3.4.3",
"@embroider/router": "2.1.6",
"@embroider/webpack": "3.2.1",
"ember-template-imports": "^4.0.0",
"ember-source": "5.5.0",
NullVoxPopuli commented 7 months ago

Do you happen to have what the compiled version of that component looks like?

johanrd commented 7 months ago

@NullVoxPopuli like this?

radial-gradient-reproduction.ts:

import { template } from "@ember/template-compiler";
const RadialGradientReproduction = template(`
  <svg viewBox="0 0 10 10" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <defs>
      <radialGradient id="myGradient">
        <stop offset="10%" stop-color="gold" />
        <stop offset="95%" stop-color="red" />
      </radialGradient>
    </defs>
    <circle cx="5" cy="5" r="4" fill="url('#myGradient')" />
  </svg>
`, {
    eval () {
        return eval(arguments[0]);
    }
});
export default RadialGradientReproduction;

(how the browser loads the component)

johanrd commented 7 months ago

btw, this seems to be an issue with all svg elements with camelCased tag names, e.g:

https://developer.mozilla.org/en-US/docs/Web/SVG/Element/animateMotion https://developer.mozilla.org/en-US/docs/Web/SVG/Element/animateTransform https://developer.mozilla.org/en-US/docs/Web/SVG/Element/clipPath https://developer.mozilla.org/en-US/docs/Web/SVG/Element/textPath

NullVoxPopuli commented 6 months ago

repro w/ error

attempts to work-around

NullVoxPopuli commented 6 months ago

This has all the tags we'd want: https://www.npmjs.com/package/html-spec-tags?activeTab=readme

NullVoxPopuli commented 6 months ago

or, maybe this is an issue with getTemplateLocals (what we use to build the scope argument in strict mode):

image

chancancode commented 6 months ago

I ran into this too.

I didn't try super hard to break it, so this is not necessarily conclusive, but I added a test to the the strict mode tests and it passes:

  @test
  'camelCased SVG elements are not considered identifiers/references'() {
    // https://developer.mozilla.org/en-US/docs/Web/SVG/Element/linearGradient
    const Foo = defineComponent({}, stripTight`
      <svg viewBox="0 0 10 10">
        <defs>
          <linearGradient id="myGradient">
            <stop offset="5%" stop-color="gold" />
            <stop offset="95%" stop-color="red" />
          </linearGradient>
        </defs>

        <circle cx="5" cy="5" r="4" fill="url('#myGradient')" />
      </svg>
    `);

    this.renderComponent(Foo);
    this.assertHTML(stripTight`
      <svg viewBox="0 0 10 10">
        <defs>
          <linearGradient id="myGradient">
            <stop offset="5%" stop-color="gold" />
            <stop offset="95%" stop-color="red" />
          </linearGradient>
        </defs>

        <circle cx="5" cy="5" r="4" fill="url('#myGradient')" />
      </svg>
    `);
    this.assertStableRerender();
  }

So that matches my suspicion that the problem is not, strictly speaking, in glimmer-vm. I think the problem is likely that babel-plugin-ember-template-compilation is using getTemplateLocals() and that function is no good and fundamentally unsound.

chancancode commented 5 months ago

While waiting for this to be fixed, I made a few template-only components for the few SVG elements that has this problem, so I can import them into .gts files and unblock the conversion:

Screen Shot 2024-03-11 at 11 49 39 PM

(If you aren't using TypeScript you don't need a .ts/.js file for this)

On the far right is a .gts component with import clipPath from "my-app/components/svg/clip-path";.