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

Remove some outdated route method references #1822

Merged
merged 2 commits into from
Jun 3, 2022
Merged

Conversation

jenweber
Copy link
Contributor

@jenweber jenweber commented Jun 3, 2022

Closes #1754

We mostly replaced the outdated uses of transitionToRoute/replaceWithRoute, but there were a couple links hanging around. The code samples already use the service, like they are supposed to.

Only 1 reviewer is needed, from any team member.

@@ -118,7 +118,7 @@ let post = store.createRecord('post', {
let self = this;

function transitionToPost(post) {
self.transitionToRoute('posts.show', post);
this.router.transitionTo('posts.show', post);
Copy link
Member

@ijlee2 ijlee2 Jun 3, 2022

Choose a reason for hiding this comment

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

The surrounding code initially didn't make sense to me because of a false indentation. I think it'd be nice to use newer JS syntax as well, to make the code more readable.

Could we update the code block to:

// Assumed to have already injected the router and store services
const newPost = this.store.createRecord('post', {
  title: 'Rails is Omakase',
  body: 'Lorem ipsum'
});

try {
  await newPost.save();
  this.router.transitionTo('posts.show', newPost.id);
} catch (error) {
  // Handle error
}

Please note that it's better to pass the ID of a record, rather than the entire record itself. If I recall, passing the entire record will result in the same issues that can happen with the <LinkTo> component (i.e. some route hooks will be skipped, likely unintentionally).

Comment on lines +10 to +13
One of the methods is [`transitionTo()`](https://api.emberjs.com/ember/release/classes/RouterService/methods/transitionTo?anchor=transitionTo).
Calling `transitionTo()` on the router service will stop any transitions currently in progress and start a new one, functioning as a redirect.

The other one is [`replaceWith()`](https://api.emberjs.com/ember/release/classes/Route/methods/replaceWith?anchor=replaceWith) which works the same way as `transitionTo()`.
The other one is [`replaceWith()`](https://api.emberjs.com/ember/release/classes/RouterService/methods/replaceWith?anchor=replaceWith) which works the same way as `transitionTo()`.
Copy link
Member

Choose a reason for hiding this comment

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

We may be able to update the surrounding paragraphs so that the sentences flow better. To limit the scope of the work, I'd suggest that we keep the changes as is.

Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

I think, in the code block, the variable self had been introduced because the function transitionToPost is not an arrow function. Converting self.transitionToRoute to this.route.transitionTo would result in a runtime error, I think.

Can we update that entire code block to the code that I suggested?

@jenweber
Copy link
Contributor Author

jenweber commented Jun 3, 2022

I didn't even think of that. Thanks for the help! Code sample has been replaced.

Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

Thanks! ✨

@jenweber jenweber merged commit 2198a61 into master Jun 3, 2022
@jenweber jenweber deleted the jw-router-links branch June 3, 2022 13:57
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.

Update "Redirection" guide for 4.0
2 participants