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

inject the router service inside ember-router #19454

Closed

Conversation

sly7-7
Copy link
Contributor

@sly7-7 sly7-7 commented Mar 9, 2021

Related #19405 (maybe replace?)

I don't know if it's the right way, and feel maybe weird.

cc/ @Ravenstine @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2021

Conceptually this seems good to me also, does it pass tests?

What do you think @Ravenstine @snewcomer?

@sly7-7
Copy link
Contributor Author

sly7-7 commented Mar 9, 2021

Unless I missed something, they pass locally. Thanks to @Ravenstine for writing it :) in his PR.
I realize some work has been done between his PR and the master, I just had to wire the router service into the ember router

Co-authored-by: Ten Bitcomb <tenbitcomb@gmail.com>
@sly7-7 sly7-7 force-pushed the inject-router-service-in-router branch from 8b693c0 to 7ed24c0 Compare March 9, 2021 15:34
@snewcomer
Copy link
Contributor

@sly7-7 This seems like a close direct copy with some modifications. Why don't we continue with comments and iterate on @Ravenstine's PR?

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2021

Either is fine, @sly7-7 was using the Co-authored-by thing to attribute the work from @Ravenstine, but I'm not sure why it doesn't show that in the commit above. 🤔

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2021

OK, so looks like this strategy definitely works.

@Ravenstine - Think you can incorporate this into #19405?

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2021

Thanks for testing this out @sly7-7! I'm going to go ahead and close this one, in favor of landing these tweaks over in #19405.

@rwjblue rwjblue closed this Mar 9, 2021
@sly7-7
Copy link
Contributor Author

sly7-7 commented Mar 9, 2021

@snewcomer Actually, I didn't understand all the things done in #19405. At first I wanted to cherry-pick the commit of the test, but I realized it was in the same. So just by laziness and simplicity for me to try that out, I had copied the test, and I'm sorry, because I didn't realize the overall PR was so close to the other. I apologize it was seen as a copy/paste.

@snewcomer
Copy link
Contributor

That certainly is my fault for making that assumption and I sincerely apologize!

So the only difference is Ben's throws an error if the router svc isn't available whereas yours fails gracefully. We weren't sure which approach is preferred but throwing the error caught a few tests that needed to setupRouter explicitly.

https://github.com/emberjs/ember.js/pull/19405/files#r575540397

@sly7-7
Copy link
Contributor Author

sly7-7 commented Mar 10, 2021

@snewcomer There is no problem at all :) After all, we just wanted the same thing, to move this issue forward :)

I've commented in #19405, because I think there is a mistake with the isIntermediate check.

For the nullish thing, I have no idea what should be preferred, I've just adapt the code to make tests pass without changing them. But ultimately, talking with @rwjblue it seems that this https://github.com/emberjs/ember.js/pull/19454/files#diff-02300d4f8ddd433fa4571addf26231e52255efd632d0b9972b02086584bf15ccR172 should be removed. He told me it was here only because of tests.

@sly7-7 sly7-7 deleted the inject-router-service-in-router branch March 12, 2021 09:21
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

3 participants