search

docsifyjs / docsify

🃏 A magical documentation site generator.
https://docsify.js.org
MIT License
27.78k stars 5.68k forks source link

Chore: Clean up server implementation and update test docs #2316

Closed jhildenbiddle closed 11 months ago

jhildenbiddle commented 11 months ago

Summary

This PR cleans up our server configuration as discussed in #2104 and #2218. The simplified end result is:

  1. A single server dependency for local development, previews, and testing.
  2. No duplicate index.html files to maintain (/index.html and /docs/index.html)
  3. No duplicate files in root for Vercel previews (/index.html and /themes/)

Details:

Related issue, if any:

2218

What kind of change does this PR introduce?

Other

For any code change,

Does this PR introduce a breaking change?

No

vercel[bot] commented 11 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docsify-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2023 7:14pm
jhildenbiddle commented 11 months ago

Thanks for the quick review and approval, @trusktr!

in https://github.com/docsifyjs/docsify/issues/2104. That's the only item from that list that this handles right?

Correct.

Looks like the deployments are not working now (I'm guessing the index.html change) and needs an update so that we can verify it works.

Ahh, yes. Hadn't considered the deployments. I'm not familiar with our Vercel setup nor do I know if I have access to change it (I'm not at my computer right now).

@sy-records? This seems like the stuff we always count on you to handle 😊. If I already have access to Vercel I can look into it when I get home. If I don't, would you mind granting access to Vercel when you have a moment?

sy-records commented 11 months ago

Need to set up Development Command?

Vercel Setting Screenshot ![IMG_8588](https://github.com/docsifyjs/docsify/assets/33931153/ef294537-73ac-4105-8c6c-d1096f9f09bf)
jhildenbiddle commented 11 months ago

Need to set up Development Command?

Can Vercel serve files using our npm serve script? If so, we should configure Vercel to do the equivalent of the following commands:

npm run build
npm run serve

If Vercel only serves static files, I will need to update the PR to accommodate. This would explain the need for the additional index.html file in the root directory. If we need to go this route, I'll come up with something more elegant that the duplicate index.html file in the root (which was confusing).

FYI: Currently the PR only supports serving the /docs directory with local build files using our own server (BrowserSync). This is because our server handles rewriting CDN URLs to local URLs, which is how I intended to use the same /docs/index.html file for both development (using local URLs) and production (using CDN URLs). It works great locally. Hopefully Vercel can accommodate.

sy-records commented 11 months ago

Okay. I got it.

Let me try.

jhildenbiddle commented 11 months ago

I did a little research and it doesn't look like Vercel will allow us to serve files via our own server / script.

We can use serverless functions to serve files with our own server (see here), but the implementation feels like a hack and will force our previews to be served under an /api directory on Vercel. Not great.

Unless there are other suggestions, I think the easiest solution is create a separate index.html file for Vercel to serve, similar to what we had before but implemented in a more elegant way. For example, we could have a build:preview script that generates the index.html file and copies all necessary files to .vercel/output/static to be served as static files at / (see here for details). Just an idea.

I'm open to other suggestions, too. 😄

Koooooo-7 commented 11 months ago

After check the docs, I have a simple redirect config in vercel #2317 . It makes the preview works fine, but the index.html url path is not much as expected tho (IMO, it does not that matter).

jhildenbiddle commented 11 months ago

All set!

Vercel preview deployments are working again. I've update the summary above to include the new changes. The changes made specifically to address the preview deployments are:

  • Add build:html script to generate /docs/preview.html from /docs/index.html
  • Add vercel.json file to handle preview redirect to /docs/preview.html
  • Remove duplicate /themes directory in root (no longer needed)
Koooooo-7 commented 11 months ago

Hi @jhildenbiddle I think we have different thoughts about the remove root index.html change here. 😅

I thought we gonna remove the duplicated index.html, then, we use the /docs/index.html with local path imports (/lib/docsify.js...etc) which could benefit us to dev and test locally.

About the pages and preview issues, we could enhance CI stuff to do adoptions, thats why I try to config vercel and custom github pages flow.

In current changes, we delete the root /index.html but generated a /docs/preview.html to support the vercel-preview and local dev. I'm puzzled now. Because the original /index.html can do it already, why should we remove it and instead create preview.html? It just change to dynamic generate the preview.index at dev mode on every build instead of putting some for dev index.html at root path directly (the /index.html).

If we decide use this "preview.html way", I think we could do some refine as well.

If we decide to do those thing in CI part, I could move on in the https://github.com/docsifyjs/docsify/pull/2317.

WDYT?

jhildenbiddle commented 11 months ago

Apologies for the slow response, @Koooooo-7. I got pulled away on some other projects.

In order to keep things move on, I think we could approve the changes and left my questions, it could be discussed and do the refinements later.

Perfect. This is exactly what I was going to suggest. :)