Don't call customizeComponentName on curly components
#1255
Conversation
| // If the name in question is an uppercase (i.e. angle-bracket) component invocation, run | ||
| // the optional `customizeComponentName` function provided to the precompiler. | ||
| if (resolution.resolution() === SexpOpcodes.GetFreeAsComponentHead && isUpperCase(name)) { |
There was a problem hiding this comment.
We chatted a bit about this in discord, but I think even after this change we still have an issue. I would expect (for example) that {{#Foo}}{{/Foo}} should also not call customizeComponentName, prior to #1253 we only attempted transformation for element.tag specifically.
| test('customizeComponentName is not invoked on curly components', function (assert) { | ||
| let wire = JSON.parse( | ||
| precompile('{{#my-component}}hello{{/my-component}}', { | ||
| customizeComponentName(input: string) { | ||
| return input.toUpperCase(); | ||
| }, | ||
| }) | ||
| ); | ||
|
|
||
| let block: WireFormat.SerializedTemplateBlock = JSON.parse(wire.block); | ||
|
|
||
| let [[, componentNameExpr]] = block[0] as [WireFormat.Statements.Block]; | ||
|
|
||
| glimmerAssert( | ||
| Array.isArray(componentNameExpr) && | ||
| componentNameExpr[0] === SexpOpcodes.GetFreeAsComponentHead, | ||
| `component name is a free variable lookup` | ||
| ); | ||
|
|
||
| let componentName = block[3][componentNameExpr[1]]; | ||
| assert.equal(componentName, 'my-component', 'original component name was used'); | ||
| }); |
There was a problem hiding this comment.
Can you make another (I think failing) test that looks like:
And also confirms that customizeComponentName is not called? I believe that the current implementation will fail a test like this, but prior to #1253 would have passed.
There was a problem hiding this comment.
Looks like you're right: https://github.com/dfreeman/glimmer-vm/pull/1/checks?check_run_id=1772791358#step:5:296
There was a problem hiding this comment.
Done (thanks @jamescdavis!)
|
Thanks for the review, @rwjblue! Chatting with @pzuraq, it sounds like this is going to be a bit more complex to completely restore the previous behavior. We generally treat a green |
|
The issue is that reverting #1253 also leaves us in a bad state, the following is broken (and was the original reported issue in emberjs/ember.js#19334): Prior to #1253 this snippet would assume that |
|
Also, to be super clear here: We absolutely must fix this prior to an [email protected] release. I think @pzuraq is going to try to poke at it a bit more today, hopefully he can spot the right path forward. |
|
@dfreeman / @jamescdavis - Would one y'all mind opening an issue on the Ember side for this also? I would love to make sure that there is something that others running into issues in the 3.25.0-beta series could find upon a search over there (since not everyone understands the repo relationships). |
|
I fixed up #1256 by making the no conflict test a todo, just for illustrative purposes (or, you know, if all else fails). |
pzuraq
force-pushed
the
leave-blocks-alone
branch
from
fe3bf18
to
5c0cf6c
Compare
|
When this is incorporated into beta, can someone open a PR to revert ember-learn/super-rentals-tutorial@cc75067 and confirm we can get a clean build? |
The PR is still pending emberjs/ember.js#19363 |
The change in #1253 caused
customizeComponentNameto be called on block-form curly components, while before it was only invoked for angle-bracket components. For anyone making ahem questionable use of::in curly component names in Ember, this meant those would always be converted to/instead.This change restores the previous behavior and adds a test to cover it. Thanks to @pzuraq for confirming what was going on and the likely fix!