search

docsifyjs / docsify

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

v5 preparation: updates and refactors #2428

Closed jhildenbiddle closed 4 months ago

jhildenbiddle commented 4 months ago

Summary

This PR includes refactor and clean-up work in preparation for the next major release (v5). Only changes to build and docs are included. No functional or test-related changes have been made aside from those required to facilitate the new build output directory (see below).

💡Due to the number of files affected, it may be easier to review individual commits instead of the resulting file change.

Separate but related to this PR:

  1. Deciding if/how we want to handle CDN URLs out in the wild that will automatically redirect to the /lib directory of the latest Docsify versions.
  2. Required updates to docsify-cli to align with these changes.

These are separate discussions and will be handled in a separate PRs before the release of v5.

Related issue, if any:

2336

What kind of change does this PR introduce?

Refactor Docs Build-related changes

For any code change,

Does this PR introduce a breaking change?

No

Tested in the following browsers:

vercel[bot] commented 4 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 May 21, 2024 6:14am
trusktr commented 4 months ago
  • Refactor CSS builds from /build scripts to CLI tools

Would love to remove Stylus and just use plain CSS, as it now has pretty much everything we used stylus for. Maybe it would have been a better spend of time if you just deleted Stylus build and converted to CSS?

Stylus deletion is listed here:

The more tools we can remove, the better. Most people will be fine without CSS minification, most Docsify sites are not million-user e-commerce sites. And if a team or company has that many users, then they have the skill and resources to minify source code. They can also get in touch with us and get on a sponsorship plan in exchange for optimization here:

https://github.com/sponsors/docsifyjs

(Btw we have a couple sponsors there we should give a shout out to!)

  • "engines" prop to specify node >= 20

Is it necessary? I'd only add it if a problem is encountered (did that happen?), otherwise no need to unnecessarily block someone if they're on an older version of Node.

  1. Deciding if/how we want to handle CDN URLs out in the wild that will automatically redirect to the /lib directory of the latest Docsify versions.

I think the only way is to continue to publish the old lib files in the new version for some time, but add a deprecation warnings to them (f.e. with console.warn, or maybe even going as far as injecting a small message at the bottom of the sidebar or something) telling people to stop using versionless URLs and that these files will be removed in N number of months (6 months?).

Speaking of this, how can we enforce it for v5 to v6? And idea is while we're on v5, the dist folder could be dist/v5/ and when v6 comes out then we move to dist/v6/ but keep dist/v5/ around for N months with deprecation warnings for those still not using version number. Etc.

jhildenbiddle commented 4 months ago

Would love to remove Stylus and just use plain CSS, as it now has pretty much everything we used stylus for. Maybe it would have been a better spend of time if you just deleted Stylus build and converted to CSS?

Agreed. Happy to make this change either to this PR or as a separate PR. Since this PR has already been approved by @Koooooo-7, I'd prefer we get this one merged and handle removing Stylus in a separate PR.

The more tools we can remove, the better. Most people will be fine without CSS minification, most Docsify sites are not million-user e-commerce sites. And if a team or company has that many users, then they have the skill and resources to minify source code.

Agreed on minimizing dependencies and removing unnecessary tools, but I don't we should do so at the expense of established best practices like optimizing the JS and CSS output we provide our users. It's easy to do and, more importantly, I believe it is what users familiar with these types of optimizations expect a project like Docsify to do.

  • "engines" prop to specify node >= 20

Is it necessary? I'd only add it if a problem is encountered (did that happen?), otherwise no need to unnecessarily block someone if they're on an older version of Node.

Yes. The new rollup build uses import.meta.dirname which was introduced in node 20.

I think requiring the latest LTS version of node is acceptable and non-controversial. I do not think the same is true for non-LTS versions (21) or "current" versions that have not yet entered LTS support (22).

Node v20 is the latest LTS version and all of our tooling supports it (I've already made the change from node 18 to 20 in our Vercel preview settings). Node 22 is the "current" release and will enter long-term support (LTS) in October of this year, so I don't see requiring node 20 today as an issue.

As for developers being blocked, tools like nvm make version switching version trivial and the engines prop will ensure that they receive a warning if they forget to switch to >= v20. If/when we encounter a maintainer / contributor blocked by the node 20 requirement, I'd lean towards getting them setup with a tool like nvm rather than limiting our access to features available in the latest LTS version of node.

  1. Deciding if/how we want to handle CDN URLs out in the wild that will automatically redirect to the /lib directory of the latest Docsify versions.

I think the only way is to continue to publish the old lib files in the new version for some time, but add a deprecation warnings to them (f.e. with console.warn, or maybe even going as far as injecting a small message at the bottom of the sidebar or something) telling people to stop using versionless URLs and that these files will be removed in N number of months (6 months?).

Yep. This is the option we discussed previously on Discord and the one I'm leaning towards. There are a few details to go over that are better handled in Discord v. PR comments. I mentioned the need to have that discussion because I didn't want maintainers to think this PR represented everything I think we need to do to release v5. It isn't. It is one of several PRs that I intend to file, but I'm trying to create them 1) in an order that makes sense and 2) with high-level consensus among the maintainers.

jhildenbiddle commented 4 months ago

@trusktr --

One last item I forget to address:

Speaking of this, how can we enforce it for v5 to v6? And idea is while we're on v5, the dist folder could be dist/v5/ and when v6 comes out then we move to dist/v6/ but keep dist/v5/ around for N months with deprecation warnings for those still not using version number. Etc.

CDNs handle this for us. jsDelivr provide access to all published versions via @ version locks. For example, @4 CDN URLs will continue to work when v5 is published and becomes the CDN redirect target or @latest. Same goes for @5 URLs when v6 is published.

Other CDNs (like bootcdn.cn) do the same using the version as path of the path. For example: https://cdn.bootcdn.net/ajax/libs/docsify/4.13.1/docsify.js

If the only outstanding issue is the removal of Stylus, let's approve this PR so we can move forward and I will remove Stylus in a separate PR (likely later tonight).

Thanks!

trusktr commented 4 months ago

Yes. The new rollup build uses import.meta.dirname which was introduced in node 20.

Good enough for me!

I'd prefer we get this one merged and handle removing Stylus in a separate PR.

SGTM

@4 CDN URLs will continue to work when v5 is published and becomes the CDN redirect target or @latest. Same goes for @5 URLs when v6 is published.

Yeah, people should use versioned URLs. Totally. What I mean is that someone out there might (for whatever reason that might not be good) use an unversioned URL like https://unpkg.com/docsify/dist/docsify.js or similar to use v5, then that could cause their app to break when we release v6.

So, the idea was that if we publish something like https://unpkg.com/docsify/dist/v5/docsify.js where the v5/ folder is actually part of what we publish, then later when we release v6 we'd just update the dist folder to dist/v6/ while leaving dist/v5/ with a the old code and deprecation warnings (delete it after some time).

Basically this would force people to have a version in their URLs, at the cost of us having some more steps during major version bumps and having to eventually delete the old folder.

Would this be worth it? Or should we not worry about helping people who may still use unversioned URLs, and we let their sites break without a warning?

jhildenbiddle commented 4 months ago

@trusktr --

Moving the versioned URL discussion to Discord.

Sine you appear to be okay with the changes in this PR, please approve it so that we unblock other PRs that are waiting on this one to be merged.

Thx!