VM Upgrade: T.S. '89 #20658
VM Upgrade: T.S. '89 #20658
Conversation
NullVoxPopuli
force-pushed
the
upgrade-glimmer-vm
branch
from
a217f2c to
fb26e0f
Compare
NullVoxPopuli
force-pushed
the
upgrade-glimmer-vm
branch
from
c238f0a to
6bce2f9
Compare
NullVoxPopuli
force-pushed
the
upgrade-glimmer-vm
branch
from
f1f4fd7 to
dd24249
Compare
|
Rebase after #20672 lands |
| @@ -2492,7 +2492,7 @@ moduleFor( | |||
| }, | |||
| value: null, | |||
| }), | |||
| template: '<div id="inner-value">{{wrapper.content}}</div>', | |||
| template: '<div id="inner-value">{{this.wrapper.content}}</div>', | |||
There was a problem hiding this comment.
Something is fishy/scary that these continued to work, seemingly without assertions/deprecations either. Hopefully the new VM code is erroring consistently now, but this means we may have implicit this code in the wild that was accidentally working
There was a problem hiding this comment.
I think expectAssertion below would have swallowed any other assertions, even if it matched on backtrackingMessageFor
| }); | ||
|
|
||
| this.assertText('Godfrey Chan'); | ||
| } |
There was a problem hiding this comment.
This went from deprecation to nothing (instead of an assertion). I don't think it's illegal per-se, I think we are technically allowed to do it, but that's when we tend to get ourselves into trouble – see the {{#with}} thing recently. Next time at least consider whether we can keep an assertion around for at least 1-2 LTS.
packages/ember-template-compiler/lib/plugins/assert-against-attrs.ts
Outdated
Show resolved
Hide resolved
packages/ember-template-compiler/lib/plugins/transform-resolutions.ts
Outdated
Show resolved
Hide resolved
packages/ember-template-compiler/lib/plugins/transform-wrap-mount-and-outlet.ts
Outdated
Show resolved
Hide resolved
packages/ember-template-compiler/tests/plugins/assert-splattribute-expression-test.js
Show resolved
Hide resolved
There was a problem hiding this comment.
I don't know what this test file is trying to test anymore 😐
This probably corresponds to some kind of transform we did here to make some of these syntax work before they were implemented in the VM (this file probably predates the VM even existing), but we don't do that anymore, and the test doesn't actually do anything other than, checking these syntax compiles without error.
Surely I hope that is already covered indirectly by any tests that actually verifies/exercises the feature. This is not that different from smoke testing <p>hi</p> and {{this.foo}} compiles without error, which, I suppose isn't inherently useless, but if we are going to do that we would want to add a more cases.
Not really your problem but I wonder if we should just delete this whole file.
There was a problem hiding this comment.
I'm happy to do that in a followup. I worry about scope creep in PRs like this
NullVoxPopuli
force-pushed
the
upgrade-glimmer-vm
branch
from
de14c5f to
31cbb58
Compare
packages/ember-template-compiler/tests/plugins/assert-splattribute-expression-test.js
Show resolved
Hide resolved
| getDebugInstance(): unknown { | ||
| return null; | ||
| } | ||
| abstract getDebugInstance(state: InternalModifierState): unknown; |
There was a problem hiding this comment.
ah, this is what you meant
There was a problem hiding this comment.
Yeah, it’s a little bit of an edge case because we are implementing an interface. If we are extending an abstract super class you wouldn’t have to explicitly propagate it like this.
chancancode
force-pushed
the
upgrade-glimmer-vm
branch
from
04a47de to
0275684
Compare
NullVoxPopuli
force-pushed
the
upgrade-glimmer-vm
branch
from
597e015 to
9268be4
Compare
We have to make a judgement call per occurance to determine what we
should return here.
This whole mess could use some simplification, but we probably just
won't end up keeping this around for much longer once/if we remove
the action modifier, which is currently the only kind of modifier
we have internally (since `{{on}}` is implemented in glimmer-vm and
essentially duplicates this whole "internal" set up once more time
over there).
NullVoxPopuli
force-pushed
the
upgrade-glimmer-vm
branch
from
9268be4 to
0f4fc4d
Compare
It wasn't a "real" Ember deprecation so didn't go through the infra that would have caused a test failure, just spewing a lot of logs to the console.
Address a missed deprecation introduced in #20658
Changelog: https://github.com/glimmerjs/glimmer-vm/releases/tag/v0.89.0
I had some issues with
this.attrs, so I just remove the problematic tests, sinceattrsin general was supposed to be removed in v4, see: #20671 for that effort.