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

Built-in macros need to be used outside a SFC #6400

Open
nomadboy20 opened this issue Aug 3, 2022 · 10 comments
Open

Built-in macros need to be used outside a SFC #6400

nomadboy20 opened this issue Aug 3, 2022 · 10 comments
Labels
✨ feature request New feature or request

Comments

@nomadboy20
Copy link

What problem does this feature solve?

Code simplicity & Code modularity & composition API usage boost

What does the proposed API look like?

Root elemnet

<template>


    <div>
        root
    </div>

    <c1 v-model="value" />


</template>


<script setup lang="ts">

import c1 from './c1.vue'

const value = ref<string>('example')

</script>

Chidld 1

<template>

    <div>
        Child 1
    </div>

    <c2 v-model="value" />

</template>

<script setup lang="ts">

import c2 from './c2.vue'
import { useComposuable } from './composuable'

const {
    value
} = useComposuable()


</script>

Child 2

<template>

    <div class="">

        <div>
            Child 2
        </div>

        <input type="text" v-model="value">

    </div>
    
</template>
<script setup lang="ts">

import { useComposuable } from './composuable'

const {
    value
} = useComposuable()


</script>

Composuable

import { defineProps, defineEmits } from "vue";

export function useComposuable() {

    const props = defineProps<{
        modelValue: string;
    }>()

    /*
     * Emits
     */
    const emit = defineEmits<{
        (e: 'update:modelValue', value: string): void
    }>()

    const value = computed<string>({
        get: (): string => props.modelValue,
        set: (value: string): void => emit('update:modelValue', value)
    })

    return {
        props,
        emit,
        value
    }

}

(yes, we can use provide/inject with computed value, but that may not be the ideal scenario every time.)

@nomadboy20 nomadboy20 added the ✨ feature request New feature or request label Aug 3, 2022
@sxzz
Copy link
Member

sxzz commented Aug 3, 2022

IMHO, it's not a good idea. If we want to support this feature, then Vue compiler should check every function (including nested functions). It should read and parse a lot of files (many files in node_modules). Even if we skip node_modules, then Vue also needs to analyze many conditions and cases. For example:

if (someCondition) {
  fn1()
} else {
  fn2()
}

function fn1() {
  defineProps()
}
function fn2() {
  defineEmits()
}

The code only has been parsed, not executed in Vue compiler. Vue can't statically analyze which macros will be invoked.
Also, as @LittleSound mentioned if the same macros were executed repeatedly, then how should Vue do? For example:

defineProps<{ foo: string }>()
defineProps<{ foo: number }>()

// So should `foo` be `string | number` or `number`?

@nomadboy20
Copy link
Author

It should throw an error, that props have been already declerated for this component. Ok but we are dealing with issue tat some components may have same props. Just want to avoid code duplicity.

@sxzz
Copy link
Member

sxzz commented Aug 5, 2022

I think the example below is better. (We should wait for the issue)

// hooks.ts
interface ComposuableProps {
  foo: string
}

function useComposuable(props) {
  console.log(props.foo)
}
// SFC <script setup>
import type { ComposuableProps } from './hooks'
interface CompProps {
  title: string
}
defineProps<ComposuableProps & CompProps>()

useComposuable(props)

We can merge and handle types with the help of TypeScript capabilities.

@jez9999
Copy link

jez9999 commented Sep 17, 2022

@sxzz I don't use TypeScript, I hate it. You can't just assume everyone's using TypeScript.

@LinusBorg
Copy link
Member

I'm not really convinced this would be a good idea as well.

@sxzz I don't use TypeScript, I hate it. You can't just assume everyone's using TypeScript.

instead of the prop Type you could expose the props object as a separate export.

// hooks.js
export const useComposableProps = {
  foo: String
}

export function useComposable(props) {
  console.log(props.foo)
}
// SFC <script setup>
import { useComposable, type useComposableProps } from './hooks'
defineProps({
 ...useComposableProps,
 title: String
})

useComposable(props)

Some more general remarks: Coupling composables to specific props and emits

  1. will lead to conflicts with similarly named props
  2. limit reusability in components that need the functionality, but do not want or need to expose a prop or event to their parent.
  3. Further increase the compiler complexity for both code and type generation.
  4. Lead to less expressive component code, similar to the problem of mixins, where it's not clear from which composable with prop is "inherited" from.

Composition API and script setup are designed to favor explicit composition, and I think that should be embraced.

So I'm basically agreeing with @sxzz. Composables should consume props and event as normal arguments and essentially be oblivious to the actual props and events that may or may not be implemented in the consuming component.

@jez9999
Copy link

jez9999 commented Sep 17, 2022

  1. the whole point, I think, is that there are some composables the whole point of which is to extract out the event emitting logic, so you'd only use it if you did want to opt in to that. As for the other stuff, well it's all subjective. You're weakening the value proposition of composables IMHO. Kind of sucks that you want to take that decision away from devs at the same time as telling them to use composables.

From my composable emits perspective, I guess I'd be looking at code like:

import { useLayout, layoutComposableEmitsEvents } from 'layout.js';
useLayout(defineEmits(layoutComposableEmitsEvents));

Probably less readable that copy/pasting the code everywhere and barely any more expressive than having the emits logic in the composable.

@LinusBorg
Copy link
Member

I get your perspective. And I would be cool with supporting this if we could make it work in a way that would not degrade DX like props inference in the template (for TS and JS users alike).

That's a big if though, because it would need some serious additional complexity in the compiler, which would now need to read, parse, and possibly recompile additional files outside of the component, whereas right now, we can deal with each file on its own.

So from my perspective, the slight difference in readability for your specific example/use case has to be weighted against that added complexity on our end, and I don't necessarily see that cost/benefit calculation to result in a net positive.

Note that I'm not closing this issue as I don't think my opinion here is the end-all of the conversation, but that's where I stand on this right now.

@jez9999
Copy link

jez9999 commented Sep 17, 2022

How come emits are dealt with as a macro and not an imported function, out of interest?

@LinusBorg
Copy link
Member

Because we need to transform it into a normal emits declaration during compilation:

<script setup>
  const emits = defineEmits(['someEvent'])
  // setup code ...
</script>

essentially is compiled to this during build:

export default defineComponent({
  emits: ['someEvent'],
  setup(_, { emits }) {
     // setup code ....
  } 
})

You can see the generated code for yourself by having a look at the JS tab on the righthand side of this playgound

A runtime import won't be able to achieve that.

@nomadboy20
Copy link
Author

I'm not really convinced this would be a good idea as well.

@sxzz I don't use TypeScript, I hate it. You can't just assume everyone's using TypeScript.

instead of the prop Type you could expose the props object as a separate export.

// hooks.js
export const useComposableProps = {
  foo: String
}

export function useComposable(props) {
  console.log(props.foo)
}
// SFC <script setup>
import { useComposable, type useComposableProps } from './hooks'
defineProps({
 ...useComposableProps,
 title: String
})

useComposable(props)

Some more general remarks: Coupling composables to specific props and emits

  1. will lead to conflicts with similarly named props
  2. limit reusability in components that need the functionality, but do not want or need to expose a prop or event to their parent.
  3. Further increase the compiler complexity for both code and type generation.
  4. Lead to less expressive component code, similar to the problem of mixins, where it's not clear from which composable with prop is "inherited" from.

Composition API and script setup are designed to favor explicit composition, and I think that should be embraced.

So I'm basically agreeing with @sxzz. Composables should consume props and event as normal arguments and essentially be oblivious to the actual props and events that may or may not be implemented in the consuming component.

I understand and understand what you wrote and it makes sense to me. This answer partially solves this issue. Please make sure that o is also in the documentation. Or add this exaple there - export it in aiport as time and pass to props. Thanks alot 🙏

@sxzz sxzz changed the title Bbuilt-in macros need to be used outside a SFC Built-in macros need to be used outside a SFC Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature request New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants