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.

Sign up for GitHub

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

Jump to bottom

feat(nuxt): allow fallback production content in <DevOnly> #20817

Merged
merged 4 commits into from May 13, 2023
Merged

feat(nuxt): allow fallback production content in <DevOnly> #20817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ebfea16
feat(dev-only): allow fallback
ineshbose May 12, 2023
a23a75f
fix: tests
ineshbose May 12, 2023
5f9b25a
fix: invoke slot
danielroe May 13, 2023
76fbd00
Merge branch 'main' into dev-only-fallback
danielroe May 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/2.guide/2.directory-structure/1.components.md
View file
Open in desktop
Expand Up @@ -336,6 +336,11 @@ The content will not be included in production builds and tree-shaken.
<DevOnly>
<!-- this component will only be rendered during development -->
<LazyDebugBar />

<!-- if you ever require to have a replacement during production -->
<template #fallback>
danielroe marked this conversation as resolved.
Show resolved Hide resolved
<div><!-- empty div for flex.justify-between --></div>
</template>
</DevOnly>
</div>
</template>
Expand Down
2 changes: 1 addition & 1 deletion packages/nuxt/src/app/components/dev-only.ts
View file
Open in desktop
Expand Up @@ -6,6 +6,6 @@ export default defineComponent({
if (process.dev) {
return () => props.slots.default?.()
}
return () => null
return () => props.slots.fallback?.()
}
})
4 changes: 2 additions & 2 deletions packages/nuxt/src/core/plugins/dev-only.ts
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ interface DevOnlyPluginOptions {
}

export const DevOnlyPlugin = createUnplugin((options: DevOnlyPluginOptions) => {
const DEVONLY_COMP_RE = /<(dev-only|DevOnly)>[\s\S]*?<\/(dev-only|DevOnly)>/g
const DEVONLY_COMP_RE = /<(?:dev-only|DevOnly)>[^<]*(?:<template\s+#fallback>(?<fallback>[\s\S]*?)<\/template>)?[\s\S]*?<\/(?:dev-only|DevOnly)>/g
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this regex won't work with the example in the updated doc and the following html due to [^<]*:

<DevOnly>
        <p>Some dev-only info</p>
        <template #fallback>
          Some prod-only info
        </template>
      </DevOnly>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm.... i forgot to submit the review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for looking at this. we should probably add the handling to TreeShakeTemplatePlugin ...

Copy link
Member Author

@ineshbose ineshbose May 13, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing! I was chaining the two regex/matches earlier (inefficient imo) but I think putting it into one (needing [^<]) caused it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using ultrahtml instead of the regex and keep the dev-only plugin ? TreeShakeTemplatePlugin won't remove ssrRender(DevOnly...) but only remove its slots that are not fallback or placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely up for using parsers over regex for safer template handling! Should it wait for #20284 as it adds ultrahtml to Nuxt dependencies first?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do it now, it won't cause any complex merge conflict 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently gone for splitting regex (in #20840) as ultrahtml could cause trade-off with performance (through safety).


return {
name: 'nuxt:server-devonly:transform',
Expand All @@ -29,7 +29,7 @@ export const DevOnlyPlugin = createUnplugin((options: DevOnlyPluginOptions) => {
const s = new MagicString(code)
const strippedCode = stripLiteral(code)
for (const match of strippedCode.matchAll(DEVONLY_COMP_RE) || []) {
s.remove(match.index!, match.index! + match[0].length)
s.overwrite(match.index!, match.index! + match[0].length, match.groups?.fallback || '')
}

if (s.hasChanged()) {
Expand Down
2 changes: 2 additions & 0 deletions test/basic.test.ts
View file
Open in desktop
Expand Up @@ -83,6 +83,8 @@ describe('pages', () => {
expect(html).toContain('Composable | star: auto imported from ~/composables/nested/bar.ts via star export')
// should import components
expect(html).toContain('This is a custom component with a named export.')
// should remove dev-only and replace with any fallback content
expect(html).toContain(isDev() ? 'Some dev-only info' : 'Some prod-only info')
// should apply attributes to client-only components
expect(html).toContain('<div style="color:red;" class="client-only"></div>')
// should render server-only components
Expand Down
8 changes: 8 additions & 0 deletions test/fixtures/basic/pages/index.vue
View file
Open in desktop
Expand Up @@ -11,6 +11,14 @@
<div>Composable | star: {{ useNestedBar() }}</div>
<DevOnly>Some dev-only info</DevOnly>
<div><DevOnly>Some dev-only info</DevOnly></div>
<div>
<DevOnly>
Some dev-only info
<template #fallback>
Some prod-only info
</template>
</DevOnly>
</div>
<div>Path: {{ $route.fullPath }}</div>
<NuxtLink to="/">
Link
Expand Down