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

[Bug]: autoExtensions bad case "./components/button/index.js" #250

Open
SoonIter opened this issue Sep 30, 2024 · 12 comments
Open

[Bug]: autoExtensions bad case "./components/button/index.js" #250

SoonIter opened this issue Sep 30, 2024 · 12 comments
Labels
🐞 bug Something isn't working

Comments

@SoonIter
Copy link
Member

SoonIter commented Sep 30, 2024

Version

rslib 0.0.8

Details

image
.
├── src
│   ├── button
│   │   └── index.tsx
│   └── index.ts
└── tsconfig.json

input

// index.ts
import button from './button';

expected behavior

import button from './button/index.mjs';

actual behavior

import button from './button.mjs';

Reproduce link

none

Reproduce Steps

none

@SoonIter SoonIter added the 🐞 bug Something isn't working label Sep 30, 2024
@SoonIter
Copy link
Member Author

By the way, in bundless mode, if cannot resolve, error message should be emitted

@fi3ework
Copy link
Member

fi3ework commented Sep 30, 2024

Does other tools support patch mainFiles in bundleless mode as of now? For bundle mode, support index in mainFields is fine. In bundleless mode, I feel it's too lose somehow.

@SoonIter
Copy link
Member Author

Does other tools support patch mainFiles in bundleless mode as of now? For bundle mode, support index in mainFields is fine. In bundleless mode, I feel it's too lose somehow.

modernjs module does not support it yet, but its autoExtensions default value is false

@fi3ework
Copy link
Member

esbuild by default could handle index mainFiles, but when the target module is externalized, it will not adding index as well.

https://esbuild.github.io/try/#YgAwLjI0LjAALS1idW5kbGUgLS1mb3JtYXQ9ZXNtIC0tZXh0ZXJuYWw6Jy4vYScAZQBlbnRyeS5qcwBpbXBvcnQgeCBmcm9tICcuL2EnCmV4cG9ydCB7IHggfQAAYS9pbmRleC5qcwBleHBvcnQgZGVmYXVsdCA1

If we wish to support this feature, we need to use getResolve in the external function to get the resolved path, but currently getResolve is not supported.

I think we can temporarily disable mainFiles in bundleless mode to avoid this too loose behavior.

@Timeless0911
Copy link
Collaborator

Same as #238, we should check whether getResolve hook in Rspack will be supported in the nearly future.

@Timeless0911
Copy link
Collaborator

We should also handle when ./folder/index.ts and ./folder.ts exists at the same time, there may also be .ts, .tsx, .jsx file existing.

@fi3ework
Copy link
Member

We should also handle when ./folder/index.ts and ./folder.ts exists at the same time

We could just follow Rspack's default order.

there may also be .ts, .tsx, .jsx file existing.

getResolve could handle all of these.

@joshwooding
Copy link

Is there an issue to follow for this? Or a quick way to catch this issue? You mentioned disabling mainFiles?

@Timeless0911
Copy link
Collaborator

Is there an issue to follow for this? Or a quick way to catch this issue? You mentioned disabling mainFiles?

You can write the full path when writing source code to avoid this issue.

- import button from './button';
+ import button from './button/index';

@joshwooding
Copy link

Is there an issue to follow for this? Or a quick way to catch this issue? You mentioned disabling mainFiles?

You can write the full path when writing source code to avoid this issue.

  • import button from './button';
  • import button from './button/index';

The hard part is finding them all :) So was wondering if there was a quick way, I'll look into it thanks!

@fi3ework
Copy link
Member

fi3ework commented Oct 23, 2024

There's not a rational way to find the /index by leveraging Rspack without getResolve function, there are some workarounds such as manually check before resolving, but that's a bypass implementation which is an alternative to mainFiles. We haven't decided the implementation as getResolve might can't be supported due to the perf consideration.

@joshwooding
Copy link

Thanks for the quick response, makes sense. I guess #251 will help flag these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants