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

fix(nuxt): resolve internal target: blank links with base #23751

Merged
merged 17 commits into from Oct 20, 2023

Conversation

Jannchie
Copy link
Contributor

@Jannchie Jannchie commented Oct 18, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Now, such a link will automatically append the baseURL.

  <div>
    <NuxtLink to="/about" target="_blank">link with target</NuxtLink>
  </div>

This should be a breaking change. because it's possible that someone could explicitly add baseURL to the to attribute to get around the bug.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@stackblitz
Copy link

stackblitz bot commented Oct 18, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@Jannchie Jannchie changed the title fix(link): target=_blank nuxt link should join baseURL fix(nuxt): target=_blank nuxt link should join baseURL Oct 18, 2023
@Mini-ghost
Copy link
Contributor

Mini-ghost commented Oct 18, 2023

Hi! I happen to be researching this issue as well. Would you like to consider some other ideas?

let href = typeof to.value === 'object' ? router.resolve(to.value)?.href ?? null : to.value || null

if (href && href.startsWith('/') && !href.startsWith('//')) {
  const _base = withLeadingSlash(withTrailingSlash(baseURL))
  if (_base !== '/' && !href.startsWith(_base)) {
    href = joinURL(_base, href)
  }
}

This way, the following examples will work as expected:

// nuxt.config.ts
export default defineNuxtConfig({
  app: {
    baseURL: '/app/'
  }
})
<template>
  <NuxtLink to="http://localhost:3000/app/about" target="_blank">
    link with target and protocol
  </NuxtLink>
  <!--
    Output:
    <a href="http://localhost:3000/app/about" rel="noopener noreferrer" target="_blank">
      link with target and baseURL
    </a>
  -->

  <NuxtLink to="//localhost:3000/app/about" target="_blank">
    link with target and starts with `//`
  </NuxtLink>
  <!--
    Output:
    <a href="//localhost:3000/app/about" rel="noopener noreferrer" target="_blank">
      link with target and baseURL
    </a>
  -->

  <NuxtLink to="/app/about" target="_blank">
    link with target and baseURL
  </NuxtLink>
  <!--
    Output:
    <a href="/app/about" rel="noopener noreferrer" target="_blank">
      link with target and baseURL
    </a>
  -->

  <NuxtLink to="/about" target="_blank">
    link with target without baseURL
  </NuxtLink>
  <!--
    Output:
    <a href="/app/about" rel="noopener noreferrer" target="_blank">
      link without target
    </a>
  -->
</template>

@Jannchie
Copy link
Contributor Author

Hi! I happen to be researching this issue as well. Would you like to consider some other ideas?

Yeah, your idea is better. I modified this pull request with reference to your implementation... Thank you!

@danielroe danielroe self-requested a review October 18, 2023 17:34
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Would you be able to add some tests to cover this?

Co-authored-by: Alex Liu <dsa1314@gmail.com>
@Jannchie
Copy link
Contributor Author

Would you be able to add some tests to cover this?

I've added the test cases!

Copy link
Member

@lihbr lihbr left a comment

Choose a reason for hiding this comment

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

LGTM! just a small concern with the checks, and some optional style suggestions for the tests :)

Thanks for making this!

packages/nuxt/src/app/components/nuxt-link.ts Outdated Show resolved Hide resolved
packages/nuxt/test/nuxt-link.test.ts Outdated Show resolved Hide resolved
packages/nuxt/test/nuxt-link.test.ts Show resolved Hide resolved
Jannchie and others added 2 commits October 20, 2023 21:21
@Jannchie Jannchie requested a review from lihbr October 20, 2023 12:33
Copy link
Member

@lihbr lihbr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

lihbr and others added 3 commits October 20, 2023 14:45
- move `useRuntimeConfig()` out of render function
- respect `trailingSlashBehavior`
- don't reimplement `hasProtocol`
@lihbr lihbr enabled auto-merge (squash) October 20, 2023 15:19
@danielroe danielroe changed the title fix(nuxt): target=_blank nuxt link should join baseURL fix(nuxt): resolve internal target: blank links with base Oct 20, 2023
@danielroe danielroe merged commit ffa6b6e into nuxt:main Oct 20, 2023
33 checks passed
@github-actions github-actions bot mentioned this pull request Oct 20, 2023
@Jannchie Jannchie deleted the fix/nuxt-link branch October 20, 2023 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NuxtLink with target="_blank" doesn't render correctly
4 participants