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: support dynamic path with alias in new URL() (fix #10597) #10743

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hershelh
Copy link
Contributor

@hershelh hershelh commented Oct 31, 2022

Description

If the path in new URL() includes both variables and alias, Vite will not resolve the alias and cause an error during runtime.

This PR resolves the alias appear in new URL() in assetImportMetaUrlPlugin.

fixes #10597

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@hershelh

This comment was marked as outdated.

@chaxus
Copy link
Contributor

chaxus commented Nov 2, 2022

I've tested a couple of times and it seems that the failed test will pass if run with the command pnpm run test-serve css-dynamic-import in CI but if run with pnpm run test-serve, it sometimes fails. I'm not sure if this is a ci-only case since I'm not able to test it locally.

I'm also confused about the purpose of the failed test since the related PR (#9970) that added it was only applied in build phase. So I removed the change of that PR and ran the test code again. It turned out that the test passed, which means the test code below is useless for the change of the PR:

test.runIf(isServe)(
  `doesn't duplicate dynamically imported css files when served with ${label} base`,
  async () => {
    await withServe(base, async () => {
      await page.waitForSelector('.loaded', { state: 'attached' })

      expect(await getColor('.css-dynamic-import')).toBe('green')
      // in serve there is no preloading
      expect(await getLinks()).toEqual([
        {
          pathname: '/dynamic.css',
          rel: 'preload',
          as: 'style'
        }
      ])
    })
  }
)

Maybe you could take a look. @patak-dev @bluwy

pnpm run test

I also encountered this problem, but I personally feel that this test is not stable enough, try several times may make more mistakes, may pass all...

@hershelh

This comment was marked as outdated.

@hershelh

This comment was marked as outdated.

@Niputi Niputi added p2-nice-to-have Not breaking anything but nice to have (priority) p3-minor-bug An edge case that only affects very specific usage (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Dec 5, 2022
@hershelh hershelh force-pushed the fix/alias-dynamic branch 2 times, most recently from 78926cb to 1330e69 Compare December 12, 2022 17:48
@@ -28,6 +28,7 @@ import { preloadHelperId } from './importAnalysisBuild'
export function assetImportMetaUrlPlugin(config: ResolvedConfig): Plugin {
const normalizedPublicDir = normalizePath(config.publicDir)
let assetResolver: ResolveFn
let aliaResolver: ResolveFn
Copy link

Choose a reason for hiding this comment

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

aliasResolver

Suggested change
let aliaResolver: ResolveFn
let aliasResolver: ResolveFn

Comment on lines +70 to +71
aliaResolver ??= config.createResolver()
const resolvedUrl = await aliaResolver(
Copy link

Choose a reason for hiding this comment

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

Suggested change
aliaResolver ??= config.createResolver()
const resolvedUrl = await aliaResolver(
aliasResolver ??= config.createResolver()
const resolvedUrl = await aliasResolver(

if (!newUrl.startsWith('.')) {
newUrl = `/${newUrl}`
}
newUrl = '`' + newUrl
Copy link

Choose a reason for hiding this comment

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

why are you making this assignment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.2.0-beta.3] new URL doesn't work with alias and dynamic path
6 participants