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

Upgrade to Glimmer VM 0.37.0 #17244

Merged
merged 2 commits into from
Dec 10, 2018
Merged

Conversation

tomdale
Copy link
Member

@tomdale tomdale commented Dec 4, 2018

The only API change in this version of the VM relates to TypeScript types for the DOM. Type signatures now use SimpleDOM types throughout, helping ensure code doesn’t inadvertently rely on DOM APIs that are not available in SSR mode.

The other change has to do with what happens when an exception is thrown during render. Currently, Glimmer has a limited capacity to recover the state of the VM when errors are thrown from an opcode. A quick-and-dirty fix has been applied to Ember to try to catch and recover from these states, but options are limited until the root issue is addressed in Glimmer VM.

This commit changes a previously passing test to be skipped. This is because the old test covered behavior that only worked in some limited scenarios, because it left the DOM in a bad state. This could lead to cases where the same DOM element would be rendered multiple times, or in the wrong location. Glimmer VM 0.37.0 does more rigorous type checking and now fails when a re-render is attempted and the DOM is left in a bad state.

After a discussion between myself, @chancancode, @krisselden and @wycats, we felt that it was okay to temporarily mark this test as skipped since the behavior it captures was not working consistently anyway. We plan to address this more robustly in the VM itself, at which time we can re-enable the test, and remove some of the more ad hoc error recovery semantics from Ember itself.

@Turbo87
Copy link
Member

Turbo87 commented Dec 4, 2018

@tomdale looks like one of the node tests is failing

@@ -655,7 +655,7 @@ const Component = CoreView.extend(
*/
readDOMAttr(name: string) {
// TODO revisit this
Copy link
Contributor

@ro0gr ro0gr Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming this comment is related to the previous type inference below(as HTMLElement), seems it can be removed now

@rwjblue
Copy link
Member

rwjblue commented Dec 5, 2018

There were some issues in CI (BrowserStack Edge 18 failures) that will be resolved with a rebase.

The only API change in this version of the VM relates to TypeScript types for the DOM. Type signatures now use SimpleDOM types throughout, helping ensure code doesn’t inadvertently rely on DOM APIs that are not available in SSR mode.

The other change has to do with what happens when an exception is thrown during render. Currently, Glimmer has a limited capacity to recover the state of the VM when errors are thrown from an opcode. A quick-and-dirty fix has been applied to Ember to try to catch and recover from these states, but options are limited until the root issue is addressed in Glimmer VM.

This commit changes a previously passing test to be skipped. This is because the old test covered behavior that only worked in some limited scenarios, because it left the DOM in a bad state. This could lead to cases where the same DOM element would be rendered multiple times, or in the wrong location. Glimmer VM 0.37.0 does more rigorous type checking and now fails when a re-render is attempted and the DOM is left in a bad state.

After a discussion between myself, @chancancode, @krisselden and @wycats, we felt that it was okay to temporarily mark this test as skipped since the behavior it captures was not working consistently anyway. We plan to address this more robustly in the VM itself, at which time we can re-enable the test, and remove some of the more ad hoc error recovery semantics from Ember itself.
@tomdale tomdale force-pushed the tomdale/upgrade-glimmer-vm-0-37-0 branch from bc7dbcf to 4fe99df Compare December 7, 2018 23:06
@rwjblue
Copy link
Member

rwjblue commented Dec 8, 2018

Looks like the CI failure is legit, though I'm not 100% sure what the right fix is. It seems related to the simple dom changes:

not ok 4 App Boot > non-escaped content
  ---
  message: "Died on test #1     at Object.<anonymous> (/home/travis/build/emberjs/ember.js/tests/node/app-boot-test.js:55:9): parent.insertAdjacentHTML is not a function"
  severity: failed
  actual: null
  expected: undefined
  stack: TypeError: parent.insertAdjacentHTML is not a function
    at NodeDOMTreeConstruction.insertHTMLBefore (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:46272:16)
    at NewElementBuilder.__appendHTML (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:47404:23)
    at NewElementBuilder.trustedContent (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:47432:19)
    at NewElementBuilder.appendDynamicHTML (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:47408:25)
    at Object.APPEND_OPCODES.add.vm [as evaluate] (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:44442:19)
    at AppendOpcodes.evaluate (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:43980:19)
    at LowLevelVM.evaluateSyscall (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:47181:22)
    at LowLevelVM.evaluateInner (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:47127:14)
    at LowLevelVM.evaluateOuter (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:47119:14)
    at VM.next (/home/travis/build/emberjs/ember.js/dist/ember.debug.js:49192:20)
  ...

You can easily run these tests via:

# start up a production server
ember s --port 0 -prod

# in another terminal run
yarn test:node

@tomdale
Copy link
Member Author

tomdale commented Dec 10, 2018

I believe the Node tests were failing because I forgot to upgrade SimpleDOM. We switched the SSR implementation from using a non-standard SimpleDOM API to using the standard insertAdjacentHTML DOM method.

Unfortunately, older versions of SimpleDOM don't expose this method. This means that FastBoot will need to update SimpleDOM as well, and newer versions of Ember won't be compatible with older versions of FastBoot.

SimpleDOM is backwards-compatible, so we can update SimpleDOM in FastBoot now and it will continue working with previous versions as well as with Glimmer VM 0.37+. We should be able to do this in a point release, so people would get it even with conservative upgrades. Not sure if we want to detect an incompatible FastBoot/SimpleDOM version in Ember and warn about it. Architecturally, Ember doesn't "know" about FastBoot, but maybe we should make an exception here to avoid developer frustration.

@rwjblue
Copy link
Member

rwjblue commented Dec 10, 2018

fastboot already allowed the latest version, but I did bump the minimum allowed version in ember-fastboot/fastboot#206 just in case.

I'm comfortable landing this now, and adding messaging for simple-dom mismatch if/when its an actual issue...

@rwjblue rwjblue merged commit a559e8f into master Dec 10, 2018
@rwjblue rwjblue deleted the tomdale/upgrade-glimmer-vm-0-37-0 branch December 10, 2018 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants