Skip to content

fix: skip fields not supported by rolldown for rolldown-vite #4747

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

Conversation

sapphi-red
Copy link
Contributor

Description

Skip fields that are not supported by rolldown when it's running with rolldown-vite.

This PR will make ecosystem-ci pass with rolldown-vite.

Linked Issues

Additional Context


Tip

The author of this PR can publish a preview release by commenting /publish below.

Comment on lines -101 to +107
// important so that each page chunk and the index export things for each
// other
preserveEntrySignatures: 'allow-extension',
// @ts-ignore skip setting it for rolldown-vite since it doesn't support `preserveEntrySignatures` yet
...(vite.rolldownVersion
? undefined
: // important so that each page chunk and the index export things for each
// other
{ preserveEntrySignatures: 'allow-extension' }),
Copy link
Contributor Author

@sapphi-red sapphi-red May 14, 2025

Choose a reason for hiding this comment

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

This part can be reverted once rolldown/rolldown#3500 lands.

@brc-dd
Copy link
Member

brc-dd commented May 14, 2025

One issue is still there:

chunk.type === 'chunk' &&

chunk.type is undefined when using rolldown-vite. So lean chunk generation is not working properly. It is used like this:

renderChunk(code, chunk) {
if (!ssr && isPageChunk(chunk as Rollup.OutputChunk)) {
// For each page chunk, inject marker for start/end of static strings.
// we do this here because in generateBundle the chunks would have been
// minified and we won't be able to safely locate the strings.
// Using a regexp relies on specific output from Vue compiler core,
// which is a reasonable trade-off considering the massive perf win over
// a full AST parse.
code = code.replace(staticInjectMarkerRE, (_, str1, str2, flag) => {
const str = str1 || str2
const quote = str[0]
return `createStaticVNode(${quote}__VP_STATIC_START__${str.slice(1, -1)}__VP_STATIC_END__${quote}, ${flag})`
})
return code
}
return null
},

If I remove that chunk.type condition it works fine. Is this expected?

const isPageChunk = (
  chunk: Rollup.OutputAsset | Rollup.OutputChunk | Rollup.RenderedChunk
): chunk is Rollup.OutputChunk | Rollup.RenderedChunk =>
  !!(
    (!('type' in chunk) || chunk.type === 'chunk') &&
    chunk.isEntry &&
    chunk.facadeModuleId &&
    chunk.facadeModuleId.endsWith('.md')
  )

The older types had:

https://github.com/rollup/rollup/blob/7536ffb3149ad4aa7cda4e7ef343e5376e2392e1/src/rollup/types.d.ts#L925-L944

Looks like now it doesn't have type set?

@sapphi-red
Copy link
Contributor Author

sapphi-red commented May 15, 2025

@brc-dd Ah, that is a bug. I've made a fix at rolldown/rolldown#4553

github-merge-queue bot pushed a commit to rolldown/rolldown that referenced this pull request May 15, 2025
<!-- Thank you for contributing! -->

### Description
This PR adds the missing `type` property to `RenderedChunk` that exists
in rollup.

https://github.com/rollup/rollup/blob/7536ffb3149ad4aa7cda4e7ef343e5376e2392e1/src/rollup/types.d.ts#L933

reported at
vuejs/vitepress#4747 (comment)

<!-- Please insert your description here and provide especially info
about the "what" this PR is solving -->
@brc-dd brc-dd merged commit 4e3fce4 into vuejs:main May 15, 2025
9 checks passed
@brc-dd
Copy link
Member

brc-dd commented May 15, 2025

Merged it for now. I believe the rolldown fix will be pulled automatically once rolldown-vite updates to it. Anyway it's not breaking anything major and the sites work fine even with the current rolldown-vite version.

@sapphi-red sapphi-red deleted the fix/skip-fields-not-supported-by-rolldown-for-rolldown-vite branch May 15, 2025 05:16
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 this pull request may close these issues.

2 participants