Skip to content

Remove some outdated route method references#1822

Merged
jenweber merged 2 commits intomasterfrom
jw-router-links
Jun 3, 2022
Merged

Remove some outdated route method references#1822
jenweber merged 2 commits intomasterfrom
jw-router-links

Conversation

@jenweber
Copy link
Copy Markdown
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.


function transitionToPost(post) {
self.transitionToRoute('posts.show', post);
this.router.transitionTo('posts.show', post);
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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