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

Allow enhanceApp to return a Promise #599

Closed
4 tasks done
nborko opened this issue Apr 8, 2022 · 5 comments · Fixed by #1760
Closed
4 tasks done

Allow enhanceApp to return a Promise #599

nborko opened this issue Apr 8, 2022 · 5 comments · Fixed by #1760
Labels
build Related to the build system enhancement New feature or request

Comments

@nborko
Copy link
Contributor

nborko commented Apr 8, 2022

Is your feature request related to a problem? Please describe.

I need to do some asynchronous stuff in my custom theme definition, and it would be simple if enhanceApp could return a Promise, but createApp doesn't wait for a Promise.

Describe the solution you'd like

Allow enhanceApp defined in theme/index.js to be asynchronous

Describe alternatives you've considered

TLA works in the dev server, but vitepress only builds CJS modules.

Additional context

I've forked the code and implemented it for myself, but based on the backlog of pull requests and lack of activity for this project, I'll just float the suggestion and see if someone has come up with a solution that works with the current release.

Validations

@kiaking
Copy link
Member

kiaking commented May 23, 2022

Well since VitePress is static site generator, anything that should happen async should go into config file, or do it before building the app maybe?

What is your async stuff do exactly?

@kiaking kiaking added the need more info Further information is requested label May 23, 2022
@nborko
Copy link
Contributor Author

nborko commented May 23, 2022

I am documenting a plugin that initializes asynchronously because it dynamically imports component libraries and imports and compiles SFCs on demand. Vitepress is being used to show examples inline in the documentation.

Rewriting the plugin to do exactly the opposite of that solely to run un vitepress is not desirable.

@kiaking
Copy link
Member

kiaking commented May 23, 2022

Hmmm, maybe I'm getting it but not 100%. Could you provide minimal example repo that illustrate this usecase? So that maybe someone from the community can help tackling this issue 👀

@nborko
Copy link
Contributor Author

nborko commented May 23, 2022

The source for the plugin is closed, and there are multiple asynchronous calls in the plugin initialization code. However, in the most basic case, before I even initialize the plugin, I need to read in the user configuration:

enhanceApp: async ({ app }) => {
    let config
    if (typeof window === 'undefined') {
        config = require('fs').readFileSync('docs/public/config.json', { encoding: 'utf-8' })
    } else {
        config = await fetch(withBase(`config.json?_v=${Date.now()}`)).then((response) => response.text())
    }
    app.use(await AsyncVuePlugin(config)) // AsyncVuePlugin returns a Promise
}

Everything else in the configuration pipeline can run asynchronously, so requiring enhanceApp to be synchronous seems arbitrary.

I've patched the following:

diff -ur upstream/src/client/app/index.ts vitepress/src/client/app/index.ts
--- upstream/src/client/app/index.ts    2022-05-23 09:10:38.014694600 -0500
+++ vitepress/src/client/app/index.ts   2022-05-23 09:54:13.185868700 -0500
@@ -43,7 +43,7 @@
   }
 }

-export function createApp() {
+export async function createApp() {
   const router = newRouter()

   handleHMR(router)
@@ -73,7 +73,7 @@
   })

   if (Theme.enhanceApp) {
-    Theme.enhanceApp({
+    await Theme.enhanceApp({
       app,
       router,
       siteData: siteDataRef
@@ -140,12 +140,12 @@
 }

 if (inBrowser) {
-  const { app, router, data } = createApp()
-
-  // wait until page component is fetched before mounting
-  router.go().then(() => {
-    // dynamically update head tags
-    useUpdateHead(router.route, data.site)
-    app.mount('#app')
+  createApp().then(({ app, router, data }) => {
+    // wait until page component is fetched before mounting
+    router.go().then(() => {
+      // dynamically update head tags
+      useUpdateHead(router.route, data.site)
+      app.mount('#app')
+    })
   })
 }
diff -ur upstream/src/client/app/theme.ts vitepress/src/client/app/theme.ts
--- upstream/src/client/app/theme.ts    2022-05-23 09:10:38.016694600 -0500
+++ vitepress/src/client/app/theme.ts   2022-04-08 12:48:55.185623000 -0500
@@ -11,5 +11,5 @@
 export interface Theme {
   Layout: Component
   NotFound?: Component
-  enhanceApp?: (ctx: EnhanceAppContext) => void
+  enhanceApp?: (ctx: EnhanceAppContext) => void | Promise<void>
 }
diff -ur upstream/src/node/build/render.ts vitepress/src/node/build/render.ts
--- upstream/src/node/build/render.ts   2022-05-23 09:10:38.078694400 -0500
+++ vitepress/src/node/build/render.ts  2022-04-08 12:48:55.188771000 -0500
@@ -17,7 +17,7 @@
   hashMapString: string
 ) {
   const { createApp } = require(path.join(config.tempDir, `app.js`))
-  const { app, router } = createApp()
+  const { app, router } = await createApp()
   const routePath = `/${page.replace(/\.md$/, '')}`
   const siteData = resolveSiteDataByRoute(config.site, routePath)
   router.go(routePath)

Edit: pasted diff of unsaved file /o\

@kiaking
Copy link
Member

kiaking commented May 23, 2022

Thank you for the details! OK now I'm kinda seeing the point. Seems like we can do this, though I think we need careful testing before applying the code change.

I'll set this issue as an enhancement for now, since I think this feature can be implemented after 1.0.0 release.

But if someone from the community would like to tackle this issue, it's super welcome 🙌

@kiaking kiaking added enhancement New feature or request build Related to the build system and removed need more info Further information is requested labels May 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Related to the build system enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants