Skip to content

test: resolve using absolute dep path#5495

Merged
patak-cat merged 5 commits into
vitejs:mainfrom
sanyuan0704:fix/dep-absolute-path
Apr 9, 2023
Merged

test: resolve using absolute dep path#5495
patak-cat merged 5 commits into
vitejs:mainfrom
sanyuan0704:fix/dep-absolute-path

Conversation

@sanyuan0704
Copy link
Copy Markdown
Contributor

@sanyuan0704 sanyuan0704 commented Nov 1, 2021

Closes #5494

Description

Return the result of the pre-bundle product before resolvePlugin when import absolute dep path instead of bare import. See details in issue

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.

@sanyuan0704 sanyuan0704 changed the title fix: resolve problem when using absolute dep path fix: resolve problem when using absolute dep path (fix #5494) Nov 1, 2021
Comment thread packages/vite/src/node/plugins/preAlias.ts Outdated
@sanyuan0704 sanyuan0704 force-pushed the fix/dep-absolute-path branch from 3141615 to 2a56595 Compare November 1, 2021 07:16
@bluwy
Copy link
Copy Markdown
Member

bluwy commented Nov 1, 2021

It would also be great if we can add a test for this. I think the nested-dep playground would fit, with a test for absolute path in the vite config optimizeDeps.include. I think the path can be retrieved via path.resolve too instead of a hardcoded string.

@sanyuan0704
Copy link
Copy Markdown
Contributor Author

It would also be great if we can add a test for this. I think the nested-dep playground would fit, with a test for absolute path in the vite config optimizeDeps.include. I think the path can be retrieved via path.resolve too instead of a hardcoded string.

But the path will be used in runtime code, such as import xxx from 'absolute-path'.

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Nov 1, 2021

But the path will be used in runtime code, such as import xxx from 'absolute-path'.

Ahh you're right. Hmm this one's hard to test, I'm fine if we leave it out for now as long as we add a comment in the code to explain the motivation (as you've proposed).

@sanyuan0704
Copy link
Copy Markdown
Contributor Author

But the path will be used in runtime code, such as import xxx from 'absolute-path'.

Ahh you're right. Hmm this one's hard to test, I'm fine if we leave it out for now as long as we add a comment in the code to explain the motivation (as you've proposed).

We can execute a script in npm script, such as replace.js.

scripts: {
  "dev": "node replace.js && vite"
}

Then replace the outlet path with real absolute path.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 1, 2021
@Shinigami92 Shinigami92 linked an issue Nov 1, 2021 that may be closed by this pull request
7 tasks
Comment thread packages/vite/src/node/plugins/preAlias.ts Outdated
@bluwy
Copy link
Copy Markdown
Member

bluwy commented Nov 1, 2021

We can execute a script in npm script, such as replace.js.

scripts: {
  "dev": "node replace.js && vite"
}

Then replace the outlet path with real absolute path.

I think it's a bit brittle to do this (and may affect CI?). If someone run tests locally too, git would show a change for that file which could be confusing and lead to accidental updates.

@patak-cat
Copy link
Copy Markdown
Member

Couldnt you use a plugin to rewrite a placeholder for the import path, or even use define directly?

@sanyuan0704
Copy link
Copy Markdown
Contributor Author

Couldnt you use a plugin to rewrite a placeholder for the import path, or even use define directly?

Thanks,i will use resolve.alias replace the path.

@Shinigami92
Copy link
Copy Markdown
Member

You need to run pnpm install in the root folder to regenerate lock file

@sanyuan0704 sanyuan0704 force-pushed the fix/dep-absolute-path branch 8 times, most recently from c7172b5 to 99c0a3e Compare November 1, 2021 10:34
@sanyuan0704
Copy link
Copy Markdown
Contributor Author

You need to run pnpm install in the root folder to regenerate lock file

@bluwy @patak-js @Shinigami92 @daaku Hello, I'm not clear why does CI test always fail in windows environment.Can you help?

Comment thread packages/playground/nested-deps/index.html Outdated
Comment thread packages/playground/nested-deps/vite.config.js Outdated
@patak-cat
Copy link
Copy Markdown
Member

I think it is better if you use define instead here

import F from __F_ABSOLUTE_PACKAGE_PATH__

And in config use https://vitejs.dev/config/#define

define: {
  __F_ABSOLUTE_PACKAGE_PATH__: `JSON.stringify("${packageFPath}")`
}

If you are going to use an alias (that I think should also work), better to also use a constant name like the one above. Maybe Windows is having issues with the { char.

But for Windows you are probably missing a normalizePath, I'll comment in the code

Comment thread packages/vite/src/node/plugins/preAlias.ts Outdated
@sanyuan0704 sanyuan0704 force-pushed the fix/dep-absolute-path branch from 99c0a3e to df76e7b Compare November 1, 2021 12:06
@sanyuan0704 sanyuan0704 force-pushed the fix/dep-absolute-path branch 3 times, most recently from 55867c0 to bcb1c41 Compare November 1, 2021 14:13
Comment on lines +1 to +2
// extract the package absolute path
const packageFPath = require.resolve('test-package-f').replace(/(\/|\\)index.js/, '')
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 need to apply normalizePath here as well so that the server._optimizeDepsMetadata?.optimized[normalizePath(id)] expression below matches.

This brings up the edgecase as well, that we need to normalize the file path for optimizeDeps.include. I'm not sure if it's something we want, because otherwise we would need to omit the normalizePath usage in server._optimizeDepsMetadata?.optimized[normalizePath(id)], but that seems to cause CI errors too? I can't find the logs for when we had the code for that.

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.

This is tricky, I'm not seeing consistent normalization in config options. Should assetsInclude be normalized? If you want it to work in both posix and windows, then all harcoded options should be normalized to posix, but if you are getting one with resolve, it may work without normalization. I'll ask in the next team meeting what is expected here. One option would normalize both when testing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In windows, normalized path will be like D_home_vite_test and if such a path is placed in the include, esbuild will not be able to resolve it.

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.

No, normalized paths using normalizePath only swap the windows \ by POSIX /

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@patak-js I have added leading slash in the windows environment and now CI has succeeded。

Comment thread package.json Outdated
@sanyuan0704 sanyuan0704 force-pushed the fix/dep-absolute-path branch 2 times, most recently from 570969f to da22e20 Compare November 2, 2021 00:16
@sanyuan0704 sanyuan0704 force-pushed the fix/dep-absolute-path branch 2 times, most recently from 6c4b117 to 37ae5b2 Compare November 2, 2021 06:50
patak-cat
patak-cat previously approved these changes Apr 8, 2023
Copy link
Copy Markdown
Member

@patak-cat patak-cat left a comment

Choose a reason for hiding this comment

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

Merged main removing the code change and only leaving the test cases, and these seem to work already without any modifications. I think we can merge this as a test PR

@patak-cat patak-cat changed the title fix: resolve problem when using absolute dep path (fix #5494) test: resolve using absolute dep path Apr 8, 2023
Comment thread playground/nested-deps/vite.config.js Outdated
Comment thread playground/nested-deps/vite.config.js Outdated
@patak-cat patak-cat merged commit 66ec31c into vitejs:main Apr 9, 2023
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.

Pre-bundle problem when the dependency is an absolute path

6 participants