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

[BUGFIX beta] IE8 fixes for beta #5609

Merged
merged 2 commits into from Sep 27, 2014
Merged

[BUGFIX beta] IE8 fixes for beta #5609

merged 2 commits into from Sep 27, 2014

Conversation

mixonic
Copy link
Sponsor Member

@mixonic mixonic commented Sep 19, 2014

6 failures left to go. Everything major seems to be locked down, though this also requires that tildeio/htmlbars#91 be merged and the dependency in Ember be updated.

  • Update the dependency to HTMLBars
  • Triage the final 5 failure with @rwjblue, fix anything remaining
  • Make Travis happy

Review of this and the HTMLBars PR very welcome.

@@ -3,9 +3,9 @@ import { Renderer } from "ember-metal-views";
var renderer;

function MetalRenderer () {
MetalRenderer.super.call(this);
MetalRenderer._super.call(this);
Copy link
Member

Choose a reason for hiding this comment

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

can we do the fast no-rebind + inline version of this?

function MetalRenderer() {
  this._super$constructor();
}

MetalRenderer.prototype._super$constructor = Renderer

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This is a test helper, not even a lukewarm path. I'd like to lean toward the least surprising thing in this case. If this is a pattern that would be good to adapt across the board for internals I can make a big patch for it over the codebase

@mixonic
Copy link
Sponsor Member Author

mixonic commented Sep 23, 2014

Refactored to avoid calling morph creation helpers with the contextual element in addition to the parent (which is likely the contextual element), except where required. This meant making the morph creation helpers fail harder and faster. I'm happy with the tradeoff.

Rebased and down to five failures:

The first three of which are something obscured by wrapInTryCatch:

@@ -46,6 +46,9 @@ all the tests by not specifying a `package` param:
You can also pass `jquery=VERSION` in the test URL to test different
versions of jQuery.

To run the tests in a browser that requires ES3 recast (IE8), you must
Copy link
Member

Choose a reason for hiding this comment

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

npm start (which is the documented way to run a test server) and npm run build (the documented way to build) runs in the production environment already.

Do we need to add/change this?

Copy link
Member

Choose a reason for hiding this comment

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

we could probably add npm run-script production-server

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

lgtm. dropped this commit.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Sep 24, 2014

What what metal-views is passing IE8? Cats and dogs living together? Free sundae with with the daily special?

Only Travis stands alone in defiance of my failure-crushing drive. Running with with extra verbosity here:

https://travis-ci.org/emberjs/ember.js/builds/36110198

Also bumps HTMLBars.

During a renderTree call, look to the morphs for information about the
contextualElement. Pass this to the Ember renderer, which in turn passes
it to the buffer for use when generating an element.

Additionally, strip omitted start tags even if the child node causing
their generation is after a script tag. See the long comment in
`render_buffer`.
@mixonic
Copy link
Sponsor Member Author

mixonic commented Sep 27, 2014

I believe travis is failing because of the second commit here, the test cleanups. I've seen the first commit build successfully a few times. For example.

Trying it again with a force push.

@rwjblue
Copy link
Member

rwjblue commented Sep 27, 2014

Looks good to me, and Travis is green.

🤘 :shipit:

rwjblue added a commit that referenced this pull request Sep 27, 2014
[BUGFIX beta] IE8 fixes for beta
@rwjblue rwjblue merged commit 2459262 into emberjs:master Sep 27, 2014
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.

None yet

5 participants