Skip to content

Update Ember.js #208

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

Merged
merged 65 commits into from
Nov 9, 2015
Merged

Update Ember.js #208

merged 65 commits into from
Nov 9, 2015

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Nov 3, 2015

This PR aims to continue the Ember.js update process that was started in #152. So far I've only merged the changes from the current master branch into the existing updates branch.

/cc @stefanpenner @alexcrichton

resolves #156

stefanpenner and others added 30 commits May 6, 2015 21:14
* clear API
* render will go away in 2.0
* more re-usable (less side-effects)
# Conflicts:
#	app/components/user-link.js
#	app/controllers/search.js
#	app/routes/crate.js
0.2.x is failing to install on OSX due to broken node-sass dependency
No need to install the "ember-cli" package globally here and this will make sure that the correct version is used
@stefanpenner
Copy link
Contributor

Oh awesome, I kept meaning to come back to this...

I can provide ember related feedback, but @alexcrichton is likely the one that will have to provide feedback re: login and so forth.

@stefanpenner
Copy link
Contributor

@alexcrichton if you have some cycles to help get this to 🚢 that would be awesome.
I would love to continue to help improve (add tests) etc this app, with this landing it will restore community ability to do so.

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 5, 2015

@alexcrichton I guess the best way to figure out if there are any more issues is deploying this branch on the staging server for a few days. Who would I need to talk to to make this happen?

@alexcrichton
Copy link
Member

Reading the description of the GitHub OAuth process I don't think that I will be able to test the authenticated application state in any way locally. Any ideas?!

I had to set up a local dummy app in GitHub and set the callback URL to http://localhost:4200/authorize/github, after that I was able to develop locally (using the secret/token that GitHub provided)

I'll check this out locally and poke around, thanks for picking this up @Turbo87!

@alexcrichton
Copy link
Member

OK, poking around, here's some things I found:

@dirk
Copy link
Contributor

dirk commented Nov 6, 2015

@Turbo87: Thank you so much for putting in the effort on updating. Had a hell of a time setting things up a few days ago with all these (relatively) ancient versions of Ember and other packages.

This fixes the missing lines between results
It looks like there is a bug in the latest ember-data release which caused the crate versions to not be resolved.

Fixes the "Version null does not exist" problem
@Turbo87
Copy link
Member Author

Turbo87 commented Nov 6, 2015

@alexcrichton thanks for the quick feedback. I've pushed some more commits to fix the mentioned problems.

@alexcrichton
Copy link
Member

Alright everything checks out for me at least, so I'm gonna go ahead and merge, thanks so much again @Turbo87!

alexcrichton added a commit that referenced this pull request Nov 9, 2015
@alexcrichton alexcrichton merged commit 7374243 into rust-lang:master Nov 9, 2015
@Turbo87
Copy link
Member Author

Turbo87 commented Nov 9, 2015

awesome, thanks! 🎉

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 9, 2015

let me know if any more issues come up

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 9, 2015

@alexcrichton is crates.io updated automatically or do you have to update it manually?

@alexcrichton
Copy link
Member

I've gotta update it manually and I hope to do so either later today or tomorrow. I've got a few more Rust-related updates I'd like to land as well.

@stefanpenner
Copy link
Contributor

Woooohoo!

@alexcrichton
Copy link
Member

OK, I managed to deploy this to staging, and it looks like there's only one bug I can find so far.

For this page, https://staging-crates-io.herokuapp.com, if you click on a crate, go back to the home page, and then click on the ssh2 crate you'll see the "version null does not exist" error:

https://i.imgur.com/zhNVekU.png

Other than that it looks like it all checks out though!

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 11, 2015

For this page, https://staging-crates-io.herokuapp.com, if you click on a crate, go back to the home page, and then click on the ssh2 crate you'll see the "version null does not exist" error:

I don't seem to be able to reproduce it following you instructions. I've seen it occasionally flash and then go away again on my mobile phone but I thought it might have been the slow connection.

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 11, 2015

and just after writing this he discovers how to reproduce it... 🙈

  1. click on a crate
  2. click on the back button of the browser
  3. click on the exact same crate again

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 11, 2015

I've debugged this to the state where I know what's going on and how to fix it but I'm not sure if it's a bug in ember-data or our own code:

click on a crate

  • the versions relationship of the crate is loaded (async + hasMany)

click on the back button of the browser

  • /summary is loaded and returns lists of crates with versions: null
  • versions: null is normalized into an empty array and pushed into the store which deletes the versions downloaded in the previous step

click on the exact same crate again

  • since the async relationship is resolved the versions are not downloaded again
  • max_version can't be found in the now empty versions relationship and the error is shown

From what I remember null fields should not update any values in the store but it seems that this is not true for relationships. I tend to think that this is actually a bug in ember-data but I'm not sure.

@stefanpenner @bmac any ideas whats going on here?

as a hotfix we can check if crate.versions == null and delete crate.versions before the normalize() method is called, but I'm not sure if that is actually the best way to procede here...

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.

Pagination numeric links at bottom of reverse_dependencies don't work
4 participants