naive-ui icon indicating copy to clipboard operation
naive-ui copied to clipboard

Missing "./es/radio/src/use-radio" export in "naive-ui" package (regression from version 2.32.1)

Open Lehoczky opened this issue 2 years ago • 4 comments

TuSimple/naive-ui version (版本)

2.32.2

Vue version (Vue 版本)

3.2.37

Browser and its version (浏览器及其版本)

Chrome latest

System and its version (系统及其版本)

Windows 10

Node version (Node 版本)

16

Reappearance link (重现链接)

https://github.com/Lehoczky/naive-ui-missing-export

Reappearance steps (重现步骤)

Run the reproduction app, and a missing export error will occur. If you downgrade naive ui to version 2.32.1 the app works.

Expected results (期望的结果)

The import should work like in previous version

Actual results (实际的结果)

vite error

Missing "./es/radio/src/use-radio" export in "naive-ui" package

Remarks (补充说明)

This issue is probably caused by https://github.com/tusen-ai/naive-ui/pull/3410.

Using the use-radio composable is very important for making custom radio buttons that work with the NRadioGroup component, that's why I don't think this functionality should be removed.

This is also a breaking change, which shouldn't be in introduced in a patch release (but I guess this wan unintentional). Another issue is there might be other imports that broke because of the linked PR.

related: #3590

Lehoczky avatar Aug 26 '22 08:08 Lehoczky

I didn't know adding exports field in package.json has such effect, so it's released in a patch version, although I don't expect user to use useRadio directly.

However I think useRadio is a internal implementation so that using is unexpected. For example is somedays later useRadio may not exist due to refactor and it won't be recognized as a breaking change.

07akioni avatar Aug 27 '22 17:08 07akioni

Yeah, I completely understand your point. This composable is obviously not part of the public api since it isn't even documented, and exposing it would mean that you can't change it so easily.

My only point is that I think it's a great way to create custom radio buttons that work with the NRadioGroup. The example I've linked as a reproduction repo shows a very simple use case for such custom component.

At the end of the day, I completely understand why you might not want to expose this, but for now I think we should revert adding the exports field to the package.json, to avoid the breaking change.

Lehoczky avatar Aug 29 '22 08:08 Lehoczky

Yeah, I completely understand your point. This composable is obviously not part of the public api since it isn't even documented, and exposing it would mean that you can't change it so easily.

My only point is that I think it's a great way to create custom radio buttons that work with the NRadioGroup. The example I've linked as a reproduction repo shows a very simple use case for such custom component.

At the end of the day, I completely understand why you might not want to expose this, but for now I think we should revert adding the exports field to the package.json, to avoid the breaking change.

I think create a radio component is not hard if you don't care much about focus management & a11y. Is your radio group complex? The useRadio's returned value is somehow complex which is not easy to use. It's more like a headless component pattern.

07akioni avatar Aug 31 '22 13:08 07akioni

I mostly use useRadio like in the reproduction repo: to customize the appearance of a custom radio button component based on whether it is selected:

const nRadioGroup = inject(radioGroupInjectionKey, null);
const checked = computed(() => {
  return nRadioGroup.valueRef.value === props.value;
});

The advantage of this that I don't need to pass a selected prop to my components, and the radio group looks clean:

<NRadioGroup>
  <CustomRadioGroupItem label="1" value="1"></CustomRadioGroupItem>
  <CustomRadioGroupItem label="2" value="2"></CustomRadioGroupItem>
  <CustomRadioGroupItem label="3" value="3"></CustomRadioGroupItem>
</NRadioGroup>

Lehoczky avatar Sep 06 '22 07:09 Lehoczky

We will remove exports field in package.json in the next version so this won't be a stucking problem. However this should not be encouraged.

07akioni avatar Oct 03 '22 15:10 07akioni