Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLEANUP beta] Update GlimmerVM to 0.81 #19762

Merged
merged 2 commits into from Nov 10, 2021
Merged

[CLEANUP beta] Update GlimmerVM to 0.81 #19762

merged 2 commits into from Nov 10, 2021

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Sep 27, 2021

High level breaking changes:

  • removes deprecation of mutations during helper compute
  • removes deprecation of mutations during unknownProperty
  • @glimmer/integration-tests, @glimmer/manager, @glimmer/validator
    • #1330 Remove deprecated support for mutation after consumption during certain manager hooks (@snewcomer)
  • @glimmer/manager
  • @glimmer/integration-tests, @glimmer/manager

See full details here:

https://github.com/glimmerjs/glimmer-vm/releases/tag/v0.81.0

Closes #19735
Closes #19731

@rwjblue rwjblue changed the title [BUGFIX beta] Update GlimmerVM to 0.81 [CLEANUP beta] Update GlimmerVM to 0.81 Sep 27, 2021
@snewcomer
Copy link
Contributor

@rwjblue Does this PR need to be cherry-picked or copied into here?

#19735

@nlfurniss
Copy link
Contributor

I think you'll need to cherry-pick #19731 to fix some failures

@rwjblue
Copy link
Member Author

rwjblue commented Sep 29, 2021

Thanks @snewcomer / @nlfurniss - I've merged both of those PR's into this branch.

@nlfurniss
Copy link
Contributor

nlfurniss commented Sep 29, 2021

Looking into the failures and here is at least one fix:

glimmerjs/glimmer-vm#1343 (fixes 3/5 errors)

@nlfurniss
Copy link
Contributor

And this diff fixes the rest of the tests (and I believe it's correct)

diff --git a/packages/@ember/-internals/glimmer/tests/integration/components/angle-bracket-invocation-test.js b/packages/@ember/-internals/glimmer/tests/integration/components/angle-bracket-invocation-test.js
index 34a696ab5..1bb86b5c4 100644
--- a/packages/@ember/-internals/glimmer/tests/integration/components/angle-bracket-invocation-test.js
+++ b/packages/@ember/-internals/glimmer/tests/integration/components/angle-bracket-invocation-test.js
@@ -9,7 +9,7 @@ import { Component } from '../../utils/helpers';
 
 class CustomModifierManager {
   constructor(owner) {
-    this.capabilities = modifierCapabilities('3.13');
+    this.capabilities = modifierCapabilities('3.22');
     this.owner = owner;
   }
 
diff --git a/packages/@ember/-internals/glimmer/tests/integration/custom-component-manager-test.js b/packages/@ember/-internals/glimmer/tests/integration/custom-component-manager-test.js
index c4d6552f3..62e9b576a 100644
--- a/packages/@ember/-internals/glimmer/tests/integration/custom-component-manager-test.js
+++ b/packages/@ember/-internals/glimmer/tests/integration/custom-component-manager-test.js
@@ -463,6 +463,7 @@ moduleFor(
           return EmberObject.create({
             capabilities: componentCapabilities('3.13', {
               asyncLifecycleCallbacks: true,
+              updateHook: true,
             }),
 
             createComponent(factory, args) {
@@ -668,6 +669,7 @@ moduleFor(
         capabilities: componentCapabilities('3.13', {
           destructor: true,
           asyncLifecycleCallbacks: true,
+          updateHook: true,
         }),
 
         createComponent(factory, args) {

@nlfurniss
Copy link
Contributor

looks like only @test it does not resolve helpers with a . (period)' is failing

@snewcomer
Copy link
Contributor

Yep I was debugging that on the glimmer side. There are some op codes that I was tracking down in glimmer. Perhaps will make some progress but if you happen to know, would love to chat about it.

@nlfurniss
Copy link
Contributor

Yep I was debugging that on the glimmer side. There are some op codes that I was tracking down in glimmer. Perhaps will make some progress but if you happen to know, would love to chat about it.

Sadly not. Glimmer is like Swahili to me 😔

@snewcomer
Copy link
Contributor

  • from the discord channel

I'm struggling with what do to since we removed the this fallback. There is one failing test.

@test it does not resolve helpers with a . (period)

eg. {{hello.world}}

I believe the VM is in a bad state. I'm 👀 at GetFreeAsFallback. A few options I am thinking of and would love to hear your opinions.

  1. Force an error in the compiler if we can't resolve a local block variable. Basically if we can't find a component/helper/etc/local block variable, we error to tell the user as much. This is a stricter stance, but one it seems we can take now since removing the this fallback.
  2. Do not error while rendering {{hello.world}}. e.g. assertHTML('')

@rwjblue
Copy link
Member Author

rwjblue commented Nov 5, 2021

@ef4 is looking into the failing test here, to figure out what is going on. It is totally plausible that the test just isn't valid any longer, but it does seem strange that we don't get a better assertion / failure message.

@ef4
Copy link
Contributor

ef4 commented Nov 5, 2021

After investigating I think the test failure is revealing a legitimate bug in glimmer-vm. The GetFreeAsFallback opcode is broken.

Here's a PR to try and remove it entirely instead: glimmerjs/glimmer-vm#1362

@ef4
Copy link
Contributor

ef4 commented Nov 5, 2021

With the above glimmer-vm PR, there's still an exception in that test, but now it's a proper error and not a VM crash:

Error: Attempted to resolve a value in a strict mode template, but that value was not in scope: hello

I think the behavior is now correct, although the wording of the error message is misleading. When it was written, it could probably only be reached in strict mode. But now that the this fallback is removed it can be reached in certain positions even in loose mode. In this case, given that hello is a free variable, this:

{{hello.world}}

supports only strict resolution, since there's nowhere else it can come from, because we never supported globals resolution of this syntax.

@rwjblue
Copy link
Member Author

rwjblue commented Nov 8, 2021

Note: I'm working on rebasing now that glimmer-vm 0.83.1 is released

@rwjblue
Copy link
Member Author

rwjblue commented Nov 8, 2021

CI will fail until #19830 lands

…g features)

* Remove usages of `deprecateMutationsInTrackingTransaction`
* Remove manager-capabilities deprecations
* Remove mutation after consumption deprecations id: `autotracking.mutation-after-consumption`
* Remove `this.` property fallback deprecations id: `this-property-fallback`

Co-Authored-By: Scott Newcomer <snewcomer24@gmail.com>
Co-Authored-By: Nathaniel Furniss <nlfurniss@gmail.com>
@rwjblue rwjblue merged commit 4eb36b9 into master Nov 10, 2021
@rwjblue rwjblue deleted the update-glimmer-vm branch November 10, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants