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

Don’t crash \w Ember 2.0 #3667

Merged
merged 6 commits into from
Aug 20, 2015
Merged

Conversation

stefanpenner
Copy link
Member

There may be more, but this at-least makes my one app work correctly.

  • instead of poluting the Ember object, we should instead add a level of indirection by moving the polifills to a local module, and doing the funky work there.

Goal:

to ease upgrade pain for people

@stefanpenner
Copy link
Member Author

this seems to be failing related to some build tool stuff.

@ghost
Copy link

ghost commented Aug 18, 2015

Got the build working again: stefanpenner#8.

Will open another PR for demonstration purposes.

@stefanpenner
Copy link
Member Author

@mkorfmann awesome thanks.

@bmac / @igorT / @fivetanley / @wecc need anything else on this?

@ghost
Copy link

ghost commented Aug 18, 2015

I was a a bit too quick, some tests seem to be failing.

@ghost
Copy link

ghost commented Aug 18, 2015

Currently investigating...

@ghost
Copy link

ghost commented Aug 19, 2015

All tests pass, except for this:

383) DS.DebugAdapter: global failure Error: Could not find module ember-data/ext/ember/array

Any clues?

Adding fuel to the fire, this error is only appearing when running the tests in the terminal, no problems running the tests in the browser.

@stefanpenner
Copy link
Member Author

i'll take a look now.

@stefanpenner
Copy link
Member Author

Adding fuel to the fire, this error is only appearing when running the tests in the terminal, no problems running the tests in the browser.

which version of phantom are you using?

2.0 + 1.9 appear to work fine for me.

@ghost
Copy link

ghost commented Aug 19, 2015

I'm using 1.9 as well. Haven't tried with 2.0.

@stefanpenner
Copy link
Member Author

I'm using 1.9 as well. Haven't tried with 2.0.

which patch level?

@ghost
Copy link

ghost commented Aug 19, 2015

Good question, I'm not sure how I can obtain the patch level information from my current pjs installation.

But the travis build threw the same error: https://travis-ci.org/emberjs/data/builds/76207796.

@ghost
Copy link

ghost commented Aug 19, 2015

Thats what I got from running dpkg -s phantomjs | grep Version: Version: 1.9.0-1.

@stefanpenner
Copy link
Member Author

locally 1.9.2 1.9.8 and 2.0.0 appear to work..
hmm

@ghost
Copy link

ghost commented Aug 19, 2015

Weird, I'm having the same issue with pjs 2.0.0 as well.

Probably not a phantomjs issue.

@bmac
Copy link
Member

bmac commented Aug 20, 2015

@stefanpenner I just noticed the v1.13 branch was extremely out of date from the release branch. I just synced them do you mind rebasing this pr?

@stefanpenner
Copy link
Member Author

@bmac rebased

found the test failure, working on it. (its prod mode only fairly)

@stefanpenner
Copy link
Member Author

i think i got it, lets see what travis has to say.

@wecc
Copy link
Contributor

wecc commented Aug 20, 2015

🎉

@ghost
Copy link

ghost commented Aug 20, 2015

Nice!

@ghost
Copy link

ghost commented Aug 20, 2015

That was a tough one... At least for me :).

@stefanpenner
Copy link
Member Author

@mkorfmann ya, so. I realized the the prod builds of ember-data tests don't have module access to ember-data itself (they should but its just a quirk of the more aggressive code folding used in prod)

@stefanpenner
Copy link
Member Author

alright looks like this is good to go.

@ghost
Copy link

ghost commented Aug 20, 2015

@stefanpenner Good to know, thanks for explaining.

bmac added a commit that referenced this pull request Aug 20, 2015
@bmac bmac merged commit 0b33a22 into emberjs:v1.13 Aug 20, 2015
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