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

Have a discussion about the 3 different component naming conventions #94

Closed
shairez opened this issue Jan 25, 2023 · 5 comments · Fixed by #136
Closed

Have a discussion about the 3 different component naming conventions #94

shairez opened this issue Jan 25, 2023 · 5 comments · Fixed by #136

Comments

@shairez
Copy link
Contributor

shairez commented Jan 25, 2023

Reference:

Example 1 - 
<Select.Root>
	<Select.Trigger>
    <Select.Item>

Example 2:
<Select>
	<SelectTrigger>
   <SelectItem>

Example 3:
<Select>
   <Select_Trigger>
   <Select_Item>
@Yusei-0
Copy link

Yusei-0 commented Jan 27, 2023

I think the best option is example 2

@shiroinegai
Copy link
Contributor

With respect to Example 1, I came across this while looking into whether it could be tree-shaken properly:
evanw/esbuild#1420

So if I'm understanding correctly, if we do end up going for Example 1's syntax, for tree-shaking to work correctly, imports will have to originate from the file where the component is implemented.

Currently, the Select component is imported like so:

Implementation is exported via an index file:

// packages/headless/src/index.ts aliased as '@qwik-ui/headless'
export * as Select from './components/select/select'

and then imported for usage:

import { component$ } from '@builder.io/qwik'
import { Select } from '@qwik-ui/headless'

export default component$(() => {
    return (
        <Select.Root>
            <Select.Trigger />
            <Select.ListBox />
        </Select.Root>
    )
})

For it to be tree-shaken correctly, I believe @qwik-ui/headless would have to be aliased to packages/headless/src/components and then usage would be changed slightly such that importing is done like so:

import { component$ } from '@builder.io/qwik'
// select.tsx could be renamed index.tsx for it to be terser
// will become '@qwik-ui/headless/select' instead
import * as Select from '@qwik-ui/headless/select/select'

export default component$(() => {
    return (
        <Select.Root>
            <Select.Trigger />
            <Select.ListBox />
        </Select.Root>
    )
})

Personally, I prefer Example 1 as I find it a little nicer to not have to import multiple components. A little added benefit is that as I type Select., right after the period, I would get auto-complete for all the sub-components that make up the entire Select.

@gioboa
Copy link
Contributor

gioboa commented Feb 1, 2023

Is example 1 tree-shakable and the other examples not?

@shiroinegai
Copy link
Contributor

The difference between Example 2 and 3 is really just naming. Both are tree-shakable by default as each sub-component is its own export and the usage model for both of them would look like this:

import { component$ } from '@builder.io/qwik'
import {
    SelectRoot,
    SelectTrigger,
    SelectListBox
} from '@qwik-ui/headless'
// note that it doesn't have to be re-aliased like what I elaborated previously
// but you do have to import each sub-component individually

export default component$(() => {
    return (
        <SelectRoot>
            <SelectTrigger />
            <SelectListBox />
        </SelectRoot>
    )
})

Example 1 exports the sub-components as an object which is why it needs to be imported directly from the file where it is implemented or tree-shaking doesn't work. (this is assuming I understand evanw/esbuild#1420 correctly)

@gioboa
Copy link
Contributor

gioboa commented Feb 2, 2023

I see, thanks for the clarification.
With this information I can say: I think we should use the example 2 style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants