embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
337 stars 136 forks source link

@embroider/test-setup v2 causes assertion for HTML encoded char to fail #1380

Closed jelhan closed 1 year ago

jelhan commented 1 year ago

I noticed a regression if an application renders a HTML encoded character and asserts against the character not being encoded in @embroider/test-setup v2.

I have a component, which renders ×: https://github.com/ember-bootstrap/ember-bootstrap/blob/d6cf431bf2871debf04915967271419a0c848fdb/addon/components/bs-modal/header/close.hbs#L2

I have a test, which asserts assert.dom(this.element).hasText('×');.

The assertion is working fine in non-Embroider builds and with @embroider/test-setup v1. But it starts to fail when upgrading to @embroider/test-setup v2: https://github.com/ember-bootstrap/ember-bootstrap/pull/1857

Asserting against the encoded HTML character works with @embroider/test-setup v2 but does not work with non-Embroider builds. At this point I'm stuck.

jelhan commented 1 year ago

I tried reproducing the issue preventing us from asserting against the HTML encoded character. But I was not able to reproduce it in a simplified setup: https://github.com/jelhan/ember-test-embroider-test-setup-with-html-encoded-char

Nevertheless here is a PR for Ember Bootstrap showing that it fails for that complex addon: https://github.com/ember-bootstrap/ember-bootstrap/pull/1912

I guess there is another thing playing into it...

simonihmig commented 1 year ago

I believe what has changed in test-setup v2 is only that it will bring in Embroider v2 instead of v1. So likely the issue is related to the new version of Embroider then!

I vaguely remember you had brought up a similar issue before, right? What was the problem then, and the fix?

jelhan commented 1 year ago

I vaguely remember you had brought up a similar issue before, right? What was the problem then, and the fix?

I mentioned that issue some weeks ago on Discord. But haven't had time to create a GitHub issue yet. Noticed it first in Ember Bootstrap around January. Not solved yet. Just haven't had time to get back to it.

jrjohnson commented 1 year ago

I'm seeing this issue with embroider v3 in our embroider tests as well. I'm unable to reproduce it in a simple app, it may have to do with the number of {{outlets}} or something else about nesting because it doesn't happen for things in application.hbs, but starts to happen at some point in the tree. I'm going to keep playing with a test app to see if I can find the failure point, but something is weird here for sure.

ef4 commented 1 year ago

This looks like a glimmer printer bug. In a quick check, it looks like parsing and re-printing a template does:

-×
+×

If I'm right, this bug happens only in components inside addons, and only when those addons have some custom AST transforms applied. Under those conditions, embroider needs to compile away the custom stuff before the app builds, so it will run the component's template through the template compiler, re-serializing to hbs after transforming.

ef4 commented 1 year ago

Might be fixed already and we're not using the right setting here: https://github.com/glimmerjs/glimmer-vm/pull/938

ef4 commented 1 year ago

This is the fix: https://github.com/emberjs/babel-plugin-ember-template-compilation/pull/20

ef4 commented 1 year ago

Fixed in babel-plugin-ember-template-compilation 2.0.3. As it's a patch release, no embroider release is needed, just make sure you let your deps float (or patch your lockfile) so you get the update.

jrjohnson commented 1 year ago

Thanks @ef4, this fixed the issue for me 🎉