fix: filePath with "#" is a directory #2432

YufJi · 2021-03-08T10:23:02Z

before, vite did not check filePah that includes '#' is adirectory; when code includes

const contains = require('es5-ext/string/#/contains')

throw error

antfu · 2021-03-13T11:08:13Z

packages/vite/src/node/plugins/resolve.ts

@@ -234,7 +234,7 @@ function tryFsResolve(
  if (postfixIndex < 0) {
    postfixIndex = fsPath.indexOf('#')
  }
-  if (postfixIndex > 0) {
+  if (postfixIndex > 0 && !fs.existsSync(fsPath.slice(0, postfixIndex + 1))) {


Should use accessSync with try-catch (reason see the comment in tryResolveFile). And maybe we can extract it to a util function.

This would only work for cases where full extensions are being used. I think the more complete fix here is:

Try resolving without removing postfixes first

If that didn't work, treat ? and # as postfixes, remove them and try resolving again.

yyx990803 · 2021-03-15T15:27:56Z

packages/vite/src/node/plugins/resolve.ts

@@ -234,7 +234,7 @@ function tryFsResolve(
  if (postfixIndex < 0) {
    postfixIndex = fsPath.indexOf('#')
  }
-  if (postfixIndex > 0) {
+  if (postfixIndex > 0 && !fs.existsSync(fsPath.slice(0, postfixIndex + 1))) {


This would only work for cases where full extensions are being used. I think the more complete fix here is:

Try resolving without removing postfixes first

If that didn't work, treat ? and # as postfixes, remove them and try resolving again.

patak-dev · 2021-03-28T07:24:28Z

This PR would fix #2737, I linked that issue.

@YufJi would you like to work on the requested changes or do you prefer others to take on the task?

YufJi · 2021-03-31T09:55:01Z

I have modified the code as yyx suggested

Shinigami92 · 2021-03-31T11:23:44Z

@YufJi There is a failing test, could you run rebuild everything locally and test it?

rm -Rf node_modules
yarn install
yarn build-vite
yarn build-plugin-vue
yarn test

Shinigami92 · 2021-04-01T17:22:42Z

@YufJi Okay tested it locally and I don't know why but ffd4c0d definitely broke it 🙁
Can you try fix it?

Seems that packages/playground/worker/__tests__/worker.spec.ts is failing, hope that helps

YufJi · 2021-04-01T18:45:16Z

@YufJi Okay tested it locally and I don't know why but ffd4c0d definitely broke it 🙁
Can you try fix it?

Seems that packages/playground/worker/__tests__/worker.spec.ts is failing, hope that helps

Hi~ help review the change code.

packages/vite/src/node/plugins/resolve.ts

Shinigami92 · 2021-04-01T20:44:23Z

@YufJi could you please update your branch

git switch main
git remote add upstream git://github.com/vitejs/vite.git
git fetch upstream
git pull upstream main
git push
git switch fix--filePath-with-'#'-is-a-direction
git merge main
# resolve conflicts if occur
git commit -m "chore: merge main into pr"
git push

or how to update a fork: https://gist.github.com/CristinaSolana/1885435

cause we are seeing something odd here. there is an optimization in another PR https://github.com/vitejs/vite/pull/2718/files#diff-9b81bb364c02eab9494a7d27a5effc400cacbffd3b8f349c192f890c37bfc83fR243-R247 that was brought in, but it isn't in your PR because we think its quite out of date

YufJi · 2021-04-01T21:13:55Z

@YufJi could you please update your branch
git pull upstream main
or how to update a fork: https://gist.github.com/CristinaSolana/1885435

cause we are seeing something odd here. there is an optimization in another PR https://github.com/vitejs/vite/pull/2718/files#diff-9b81bb364c02eab9494a7d27a5effc400cacbffd3b8f349c192f890c37bfc83fR243-R247 that was brought in, but it isn't in your PR because we think its quite out of date

okay, let me try it~

Shinigami92 · 2021-04-01T21:14:39Z

@YufJi I just updated the comment above to help sync the fork

…b.com/YufJi/vite into fix--filePath-with-'#'-is-a-direction

patak-dev · 2021-04-02T06:07:27Z

packages/vite/src/node/plugins/resolve.ts

+  let res: string | undefined
+
+  // 1、Try resolving without removing postfixes first
+  if ((res = tryResolve(file, postfix, options, tryIndex))) {


We are going to lose the optimization introduced in #2718 with this PR.

When people use imports with extensions, we should first check for an exact match before trying the list of auto-resolved extensions. Also, I think that we should first with the extensions removed to avoid two checks per file for common cases.

The order of the tryResolveFile calls should be:

file ( tryIndex: false ) file ( tryIndex: false, removing postfixes ) for each ext file+ext file+ext ( removing postfixes ) file ( tryIndex: true ) file ( tryIndex: true, removing postfixes )

I also think that if ? and # are inside the folder path and are not part of the file, we can safely remove the postfixes so we avoid the double checks. So we should check for the presence of ? and # only after the last /. This change alone should fix most issues (the other changes in this PR are still important though).

patak-dev · 2021-04-06T21:19:32Z

@hannoeru reported that Vite + pnpm v6 has package resolution issues due to the use of # in paths. Bumping the priority of this PR.

patak-dev · 2021-04-06T21:33:30Z

Great, at the end, they are going to replace # by + in pnpm, because webpack is also having issues. I'll leave the priority bump though because IMO it is still important that we get this in.

benmccann · 2021-04-09T15:48:24Z

It looks like this may possibly fix #2346 and #2118 in which case those could be added as linked issues

benmccann · 2021-04-09T15:49:06Z

Is there a test that can be added to prevent regressions?

caticodev · 2021-04-18T19:32:34Z

@knightly-bot build this

knightly-bot · 2021-04-23T03:29:27Z

🌒 Knightly build enabled, release every night at 00:00 UTC ^{(skip if no change)}

📦 npm package

patak-dev

See comment about ordering of file checks

Eazymov · 2021-05-13T12:11:43Z

Any updates on this?)

timvlaer · 2021-05-17T10:23:26Z

I wonder if it isn't enough to start scanning for '#' after the last slash as @patak-js suggested, see timvlaer@8d94d5d Do I oversee any other scenario's that are covered by this PR?

I can open a new PR from my branch or @YufJi you can take it along in this branch. Same to me.

Changing the fromIndex fixes #2346 for me.

hyrious · 2021-07-11T01:10:37Z

packages/vite/src/node/plugins/resolve.ts

-
-  let postfixIndex = fsPath.indexOf('?')
-  if (postfixIndex < 0) {
-    postfixIndex = fsPath.indexOf('#')


As #2346 (comment) said, can we just remove this line so that # are always treated as part of file path?

0xinhua · 2021-08-03T13:48:11Z

Same error while use vite, Any updates of this mr ?

WesleyKapow · 2021-08-17T15:37:56Z

🚫 Currently blocking migration from CRA to vite :'(

bowencool · 2021-08-24T12:15:59Z

Any news?

Shinigami92 · 2021-08-24T12:47:21Z

Any news?

@bowencool Yes! See #4703 @iheyunfei is working on it

weixiao-huang · 2021-09-22T11:07:03Z

Is there any update? Currently blocking migration from Webpack to vite

fi3ework · 2021-10-11T13:17:15Z

For anyone who encountered the # issue with es5-ext, here's my workaround. Ugly, but it works. 🤨

Add to Vite config resolve.alias. Maybe I'll continue to finish the PR after knowing all the context.

        {
          find: /^es5-ext\/(.*)?\/#\/(.*)$/,
          replacement: path.resolve(require.resolve('es5-ext'), '../$1/../#/$2'),
        },
        {
          find: /^.\/#$/,
          replacement: '../#/index.js',
        },

patak-dev · 2021-10-27T11:54:32Z

Thanks for the work in this PR, which was used in part by #4703, closing as it is now merged

fix: filePath with "#" is a direction

57893b5

antfu added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 13, 2021

antfu requested changes Mar 13, 2021

View reviewed changes

yyx990803 requested changes Mar 15, 2021

View reviewed changes

patak-dev linked an issue Mar 28, 2021 that may be closed by this pull request

Failed to run dev server when folder path is containing "#" #2737

Closed

patak-dev mentioned this pull request Mar 28, 2021

Error: File could not be loaded: index.html is thrown when trying to run the development server #2118

Closed

fix: optimization code

ffd4c0d

Shinigami92 marked this pull request as draft March 31, 2021 09:40

Shinigami92 requested review from patak-dev and Shinigami92 April 1, 2021 16:26

YufJi added 2 commits April 2, 2021 01:33

fix: remove useless condition

2060f21

fix: bug

0e38265

YufJi requested a review from yyx990803 April 1, 2021 18:50

Shinigami92 reviewed Apr 1, 2021

View reviewed changes

packages/vite/src/node/plugins/resolve.ts Outdated Show resolved Hide resolved

packages/vite/src/node/plugins/resolve.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

0fb5172

Shinigami92 approved these changes Apr 1, 2021

View reviewed changes

Shinigami92 requested a review from antfu April 1, 2021 19:27

Shinigami92 marked this pull request as ready for review April 1, 2021 19:27

YufJi added 3 commits April 2, 2021 09:55

Merge branch 'main' into fix--filePath-with-'#'-is-a-direction

1a6e643

chore: merge main into pr

1121768

Merge branch 'fix--filePath-with-'#'-is-a-direction' of https://githu…

388dbef

…b.com/YufJi/vite into fix--filePath-with-'#'-is-a-direction

patak-dev requested changes Apr 2, 2021

View reviewed changes

patak-dev mentioned this pull request Apr 2, 2021

Special symbols (#) cause errors when resolving dependency paths #2826

Closed

patak-dev added p4-important Violate documented behavior or significantly improves performance (priority) and removed p3-minor-bug An edge case that only affects very specific usage (priority) labels Apr 6, 2021

patak-dev requested changes Apr 23, 2021

View reviewed changes

coeit mentioned this pull request Jul 1, 2021

feat: plot actions usadellab/GeneExpressionPlots#93

Merged

Shinigami92 added the needs rebase label Jul 6, 2021

hyrious reviewed Jul 11, 2021

View reviewed changes

kristiandupont mentioned this pull request Jul 17, 2021

The # in ext-es5 breaks in Vite medikoo/d#18

Open

yyx990803 changed the title ~~fix: filePath with "#" is a direction~~ fix: filePath with "#" is a directory Jul 23, 2021

Shinigami92 mentioned this pull request Jul 30, 2021

vite fails when cjs module requires a module with "#" in its path #2346

Closed

hyf0 mentioned this pull request Aug 24, 2021

fix: consider # as a valid dir symbol (fix #4701) #4703

Merged

9 tasks

bowencool mentioned this pull request Aug 24, 2021

replace es5-ext with core-js@^3 medikoo/memoizee#123

Closed

Shinigami92 added the has workaround label Oct 11, 2021

Shinigami92 linked an issue Oct 11, 2021 that may be closed by this pull request

vite fails when cjs module requires a module with "#" in its path #2346

Closed

patak-dev closed this Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: filePath with "#" is a directory #2432

fix: filePath with "#" is a directory #2432

YufJi commented Mar 8, 2021 •

edited

Loading

antfu Mar 13, 2021

yyx990803 Mar 15, 2021

yyx990803 Mar 15, 2021

patak-dev commented Mar 28, 2021 •

edited

Loading

YufJi commented Mar 31, 2021

Shinigami92 commented Mar 31, 2021

Shinigami92 commented Apr 1, 2021

YufJi commented Apr 1, 2021

Shinigami92 commented Apr 1, 2021 •

edited

Loading

YufJi commented Apr 1, 2021

Shinigami92 commented Apr 1, 2021 •

edited

Loading

patak-dev Apr 2, 2021 •

edited

Loading

patak-dev commented Apr 6, 2021

patak-dev commented Apr 6, 2021

benmccann commented Apr 9, 2021

benmccann commented Apr 9, 2021

caticodev commented Apr 18, 2021

knightly-bot commented Apr 23, 2021

patak-dev left a comment

Eazymov commented May 13, 2021

timvlaer commented May 17, 2021

hyrious Jul 11, 2021 •

edited

Loading

0xinhua commented Aug 3, 2021

WesleyKapow commented Aug 17, 2021

bowencool commented Aug 24, 2021

Shinigami92 commented Aug 24, 2021

weixiao-huang commented Sep 22, 2021

fi3ework commented Oct 11, 2021 •

edited

Loading

patak-dev commented Oct 27, 2021

fix: filePath with "#" is a directory #2432

fix: filePath with "#" is a directory #2432

Conversation

YufJi commented Mar 8, 2021 • edited Loading

antfu Mar 13, 2021

Choose a reason for hiding this comment

yyx990803 Mar 15, 2021

Choose a reason for hiding this comment

yyx990803 Mar 15, 2021

Choose a reason for hiding this comment

patak-dev commented Mar 28, 2021 • edited Loading

YufJi commented Mar 31, 2021

Shinigami92 commented Mar 31, 2021

Shinigami92 commented Apr 1, 2021

YufJi commented Apr 1, 2021

Shinigami92 commented Apr 1, 2021 • edited Loading

YufJi commented Apr 1, 2021

Shinigami92 commented Apr 1, 2021 • edited Loading

patak-dev Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

patak-dev commented Apr 6, 2021

patak-dev commented Apr 6, 2021

benmccann commented Apr 9, 2021

benmccann commented Apr 9, 2021

caticodev commented Apr 18, 2021

knightly-bot commented Apr 23, 2021

patak-dev left a comment

Choose a reason for hiding this comment

Eazymov commented May 13, 2021

timvlaer commented May 17, 2021

hyrious Jul 11, 2021 • edited Loading

Choose a reason for hiding this comment

0xinhua commented Aug 3, 2021

WesleyKapow commented Aug 17, 2021

bowencool commented Aug 24, 2021

Shinigami92 commented Aug 24, 2021

weixiao-huang commented Sep 22, 2021

fi3ework commented Oct 11, 2021 • edited Loading

patak-dev commented Oct 27, 2021

YufJi commented Mar 8, 2021 •

edited

Loading

patak-dev commented Mar 28, 2021 •

edited

Loading

Shinigami92 commented Apr 1, 2021 •

edited

Loading

Shinigami92 commented Apr 1, 2021 •

edited

Loading

patak-dev Apr 2, 2021 •

edited

Loading

hyrious Jul 11, 2021 •

edited

Loading

fi3ework commented Oct 11, 2021 •

edited

Loading