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

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

Merged
merged 21 commits into from
Dec 11, 2023

Conversation

jhildenbiddle
Copy link
Member

@jhildenbiddle jhildenbiddle commented Dec 2, 2023

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:

  • Modernize and standardize on a single project server by replacing live-server (no longer maintained and has outdated dependency issues) with BrowserSync which is already used for our test environments.
  • Consolidate server configurations into single file for easier management (server.config.js)
  • Update server configurations to allow multiple servers to run simultaneously:
    • Development: 3000
    • Test: 4000
    • Production: 8080
  • Update serve script to serve /docs in production mode
  • Add serve:dev script to serve /docs in development mode
  • 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
  • Fix development rendering issue caused by DocsifyCarbon.create error.
  • Add server port checking to prevent potential test/CI issues from BrowserSync's port auto-select behavior
  • Update test documentation in /test/README.md
  • Remove duplicate index.html file in root (no longer needed)
  • Remove duplicate /themes directory in root (no longer needed)
  • Remove live-server dependency

Related issue, if any:

#2218

What kind of change does this PR introduce?

Other

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

No

Copy link

vercel bot commented Dec 2, 2023

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 jhildenbiddle changed the title Chore: Replace live-server, clean up server implementation, and update test docs Chore: Clean up server implementation and update test docs Dec 2, 2023
@trusktr trusktr linked an issue Dec 2, 2023 that may be closed by this pull request
@trusktr trusktr mentioned this pull request Dec 2, 2023
17 tasks
Copy link
Member

@trusktr trusktr left a comment

Choose a reason for hiding this comment

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

Nice!

I linked this to the todo item

We have both /index.html and docs/index.html. Do we need duplicate code? Delete one.

in #2104. That's the only item from that list that this handles right?

I also linked #2218 so that'll be closed when this is merged.

@trusktr trusktr self-requested a review December 2, 2023 21:08
trusktr
trusktr previously requested changes Dec 2, 2023
Copy link
Member

@trusktr trusktr left a comment

Choose a reason for hiding this comment

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

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.

This is nice though! Nice to consolidate some duplication!

@jhildenbiddle
Copy link
Member Author

jhildenbiddle commented Dec 2, 2023

Thanks for the quick review and approval, @trusktr!

in #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
Copy link
Member

sy-records commented Dec 3, 2023

Need to set up Development Command?

Vercel Setting Screenshot

IMG_8588

@jhildenbiddle
Copy link
Member Author

jhildenbiddle commented Dec 3, 2023

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
Copy link
Member

Okay. I got it.

Let me try.

@jhildenbiddle
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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)

sy-records
sy-records previously approved these changes Dec 5, 2023
Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

Goog job!

@Koooooo-7
Copy link
Member

Koooooo-7 commented Dec 5, 2023

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.

  • How about generate the preview.html in root path and named index.html. (literally, generate the original /index.html)
  • Change the docsify.min.js to docsify.js. (good for debug)
    After those updates, we don't need the vercel.config redirection either.
    I tried a solutin on this update: changes preview. #2322, PTAL.

If we decide to do those thing in CI part, I could move on in the #2317.

WDYT?

@Koooooo-7 Koooooo-7 mentioned this pull request Dec 5, 2023
6 tasks
Koooooo-7
Koooooo-7 previously approved these changes Dec 10, 2023
Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

Update:

Separate this PR as 2 parts:

  1. Clean up server implementation. (live-server, remove /index.html)
  2. How to adapt the deploy stuff. (preview, github pages)

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.

Hi, @jhildenbiddle , plz go ahead and we can merge it asap.

@jhildenbiddle
Copy link
Member Author

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. :)

# Conflicts:
#	package-lock.json
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.

switch from live-server to five-server
4 participants