rslib icon indicating copy to clipboard operation
rslib copied to clipboard

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

Open SoonIter opened this issue 1 year ago • 7 comments

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 avatar Sep 30 '24 08:09 SoonIter

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

SoonIter avatar Sep 30 '24 08:09 SoonIter

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.

fi3ework avatar Sep 30 '24 08:09 fi3ework

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

SoonIter avatar Sep 30 '24 09:09 SoonIter

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.

fi3ework avatar Sep 30 '24 09:09 fi3ework

Same as https://github.com/web-infra-dev/rslib/issues/238, we should check whether getResolve hook in Rspack will be supported in the nearly future.

Timeless0911 avatar Sep 30 '24 09:09 Timeless0911

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.

Timeless0911 avatar Sep 30 '24 09:09 Timeless0911

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.

fi3ework avatar Sep 30 '24 09:09 fi3ework

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

joshwooding avatar Oct 22 '24 15:10 joshwooding

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';

Timeless0911 avatar Oct 23 '24 03:10 Timeless0911

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!

joshwooding avatar Oct 23 '24 09:10 joshwooding

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.

fi3ework avatar Oct 23 '24 09:10 fi3ework

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

joshwooding avatar Oct 23 '24 12:10 joshwooding