feat(nuxt): allow configuring treeshakeable composables by harlan-zw · Pull Request #19383 · nuxt/nuxt

harlan-zw · 2023-03-02T05:58:50Z

🔗 Linked issue

Not an issue but to unblock me on #19302

❓ Type of change

📖 Documentation (updates to the documentation or readme)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality like performance)
✨ New feature (a non-breaking change that adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Background

Isolating composables to a specific environment is a useful way to optimise bundles. There are many instances of code that only need to run on a single environment that are introduced through modules.

However, there isn't an easy way for modules to opt-in to this treeshaking config.

For example:

Unhead - useServerHead, useServerSeoMeta, useServerHeadSafe
nuxt-schema-org - useSchemaOrg
nuxt-og-image - defineOgImageStatic, defineOgImageDynamic, defineOgImageScreenshot
nuxt-simple-robots - defineRobotMeta
...etc

There is currently a way to handle this by wrapping code in env if blocks. This works okay, but it means the function arguments won't be tree-shaken (afaik).

You can wrap the composable call itself but this gets pretty verbose quickly and is not something module authors have control over.

export function useServerLog(input: any) {
  if (process.server) {
    console.log(input)
  }
}

<template lang="ts" setup>
// this is kept in the client build?
useServerLog({
  foo: 'bar'
  bar: 'foo',
  baz: 'bing',
  bong: 'bar',
})
// verbose!
if (process.server) {
  useServerLog({ /* */ })
} 
</template>

Solution

Make the treeShake config extendable by moving it to nuxt config.

📝 Checklist

I have linked an issue or discussion.
I have updated the documentation accordingly.

codesandbox · 2023-03-02T05:58:53Z

Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

antfu · 2023-03-03T09:33:11Z

Interesting idea! However, I have a few questions:

Should we allow both .server and .client functions to explore the same name to have different functions in different env? If so, it might need some changes on the unimport side. (could also lead to problem of function signatures not matched)
Note: This will only work with composables when you aren't using their return value. - This pitfall feels a bit too much and may lead to confusion. And honestly might not be that useful as in many cases the return value is important. It was working for Vue because we are tree-shaking mostly the hooks.
The side-effects could also lead to confusion, as from a higher view, users would see the imports in .server being executed on the client side. It can be counterintuitive.

In that sense, I think I would push back for the current implementation as I see the trade-off is not worth it. People better use onMounted or if (process.server) or dynamic import to explicitly control the boundary at this moment.

harlan-zw · 2023-03-04T01:27:27Z

Interesting idea! However, I have a few questions:

Should we allow both .server and .client functions to explore the same name to have different functions in different env? If so, it might need some changes on the unimport side. (could also lead to problem of function signatures not matched)

Note: This will only work with composables when you aren't using their return value. - This pitfall feels a bit too much and may lead to confusion. And honestly might not be that useful as in many cases the return value is important. It was working for Vue because we are tree-shaking mostly the hooks.

The side-effects could also lead to confusion, as from a higher view, users would see the imports in .server being executed on the client side. It can be counterintuitive.

In that sense, I think I would push back for the current implementation as I see the trade-off is not worth it. People better use onMounted or if (process.server) or dynamic import to explicitly control the boundary at this moment.

Thanks for the detailed review! Good points

I agree that it could be a useful feature but the limitations open the scope for too many issues. I'd like to see it in Nuxt one day but sounds like some work needs to go into Unimport first.

I've minimised the scope of this PR to only make the treeShake config extendable which I think is more reasonable.

Co-authored-by: Daniel Roe <daniel@roe.dev>

…les' into feat/server-client-only-composables

…nly-composables

danielroe

Looks great! ❤️

danielroe · 2023-03-07T09:29:35Z

Merging so @harlan-zw can go ahead with linked PR. But would particularly appreciate feedback @antfu on the schema change in this PR. If we do end up supporting something like this in unimport down the line, does it give you enough info? (it's in the preset format, so Record<packageName: string, imports: string[]>).

(Happy to change the schema before we release v3.3 if you have any suggestions for improvement.)

harlan-zw added 2 commits March 2, 2023 15:51

feat(nuxt): server/client only composables

b328d99

doc: document

2a36f07

harlan-zw added 4 commits March 2, 2023 17:17

chore: lint

1d4b278

chore: revert artifact

5649072

chore: fix ts error

6d8a273

fix: inverted logic and add test

11a4f90

harlan-zw added 3.x 🍰 p2-nice-to-have Nice to have, not blocking ✨ enhancement New feature or improvement to existing functionality labels Mar 2, 2023

harlan-zw requested a review from danielroe March 2, 2023 07:34

danielroe requested a review from antfu March 2, 2023 09:43

harlan-zw changed the title ~~feat(nuxt): server/client only composables~~ feat(nuxt): extendable treeShake config Mar 4, 2023

fix: minimise scope

eb340ad

harlan-zw added 3 commits March 4, 2023 12:37

chore: tests

1d14d14

chore: lint

e166261

chore: add doc

872928b