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

chore(next/link): do not use link for external links #5106

Merged
merged 2 commits into from
Mar 13, 2023
Merged

chore(next/link): do not use link for external links #5106

merged 2 commits into from
Mar 13, 2023

Conversation

ovflowd
Copy link
Member

@ovflowd ovflowd commented Mar 12, 2023

This PR removes next/link from external links as it has no advantage here. It also messes up with event listeners for "dynamically" changed URLs from the legacyMain script.

@ovflowd
Copy link
Member Author

ovflowd commented Mar 12, 2023

@MaledongGit, this fixes your "async/defer" change. Your change would "work" because the legacyMain script would be called after Next.js made its initial render and changes on the next/link with its custom event listeners. The solution is not to use next/link on dynamically changed URLs + external URLs.

Also, a note that in the future the legacyMain script will be removed.

@SEWeiTung
Copy link
Contributor

SEWeiTung commented Mar 13, 2023

@MaledongGit, this fixes your "async/defer" change. Your change would "work" because the legacyMain script would be called after Next.js made its initial render and changes on the next/link with its custom event listeners. The solution is not to use next/link on dynamically changed URLs + external URLs.

Also, a note that in the future the legacyMain script will be removed.

Thanks! That's what I did yesterday later and since you're going to submit this PR, I've closed mine.
It seems you got my point, and actually yours is the best way here.

I just found "async/defer" and noticed the custom event linsters' orders and that's why I changed to "async" / "defer" XD XD XD.

@SEWeiTung
Copy link
Contributor

There're some conflicts on your PR, and I've helped to change them together.

@ovflowd
Copy link
Member Author

ovflowd commented Mar 13, 2023

Thanks! That's what I did yesterday later and since you're going to submit this PR, I've closed mine.
It seems you got my point, and actually yours is the best way here.

No worries, there's no such thing as best or worst point here, you did what you believed would solve the issue, but you were almost there :)

@ovflowd
Copy link
Member Author

ovflowd commented Mar 13, 2023

There're some conflicts on your PR, and I've helped to change them together.

Thanks! Could you also review/approve the PR?

@ovflowd ovflowd added this pull request to the merge queue Mar 13, 2023
Merged via the queue into nodejs:main with commit 406726a Mar 13, 2023
@ovflowd ovflowd deleted the chore/no-next-link-for-external-links branch March 13, 2023 18:55
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.

2 participants