convert Docsify to an ES class to start modernizing the code base #1206

trusktr · 2020-06-07T23:04:06Z

This is a Draft PR (WIP). It is forked from #1189. Once that is merged, then I will change the base of this PR to develop and mark the PR as ready-for-review instead of a draft.

Summary

Start to modernize the code base. In this case I converted Docsify to class syntax. This paves the way for better intellisense in VS Code (which is already powered by TypeScript). We are then closer to moving to TypeScript if we wish to in the future.

Now if we mark any of the methods and properties with JSDoc comments, VS Code will know how to map them whenever hovering on any methods/properties of the Docsify instance (thanks to class syntax). We just need to add JSDoc comments, but that can be in a later PR.

What kind of change does this PR introduce? (check at least one)

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature
Related documents have been updated
Related tests have been updated

vercel · 2020-06-07T23:04:10Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/chyzxa7de
✅ Preview: https://docsify-preview-git-docsify-class.docsify-core.vercel.app

trusktr · 2020-06-07T23:05:25Z

There are no logic changes in this PR. Turn on "hide whitespace changes" to get a more clear picture of the change.

trusktr · 2020-06-07T23:06:28Z

test/_helper.js

-  renderMixin(proto);
-  fetchMixin(proto);
-  eventMixin(proto);
+  const { Docsify } = require('../src/core/Docsify');


we can just import the class and use it instead of re-creating it. Also makes the test more resilient in case someone updates the class but forget to update the test and the old test would still pass.

…ication site (update buble to fix an unhelpful error message)

…orts the Docsify class

* develop: Update build/css.js use a port for the tests that doesn't collide with common local server ports Revert "Updated docs site dark and light mode with switch and redesigned search bar using docsify-darklight-theme" (#1207) ci: added codesandbox build preview for PR reviews (#1193) Update cover.md (#1134) chore: update auto format config

codesandbox-ci · 2020-06-13T05:31:41Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 918574a:

Sandbox	Source
snowy-forest-pbiew	Configuration

trusktr · 2020-06-13T20:38:08Z

e2e tests failed again (flaky). Re-running them.

trusktr · 2020-06-13T20:43:14Z

src/core/index.js

@@ -1,5 +1,8 @@
 import { documentReady } from './util/dom';
 import { Docsify } from './Docsify';
+import initGlobalAPI from './global-api';
+
+initGlobalAPI();


This is a small change: I moved initGlobalAPI call from Docsify.js to index.js so that importing Docsify.js has no side-effects.

trusktr · 2020-06-13T20:44:16Z

src/core/lifecycle/index.js

@@ -0,0 +1,50 @@
+import { noop } from '../util/core';


I move lifecycle to its own mixin instead of being niside the init mixin folder. Better organization I think.

trusktr · 2020-06-13T20:45:03Z

src/core/plugin/index.js

@@ -0,0 +1,12 @@
+import { isFn } from '../util/core';


This used to be located in the same file as init mixin, so I moved it to it's own file. Better organization I think.

trusktr · 2020-06-13T20:46:09Z

src/core/render/compiler.js

+/**
+ * This class compiles the markdown of each page of a Docsify site into HTML
+ * (using some information from the provided router).
+ */


Good idea to add inline documentation me thinks. It shows up in the intellisense of your editor when hovering on the function. Make things easier to learn.

trusktr · 2020-06-13T20:47:52Z

src/core/render/index.js

+
+      // Bind event
+      this._bindEventOnRendered(activeEl);
+    }


I made literally no logic changes. I only moved the code around within the file, so that all functions are now methods inside the new classes.

jhildenbiddle

Finally got around to taking a look at this (apologies for the delay).

My feeling is that while I'm all for cleaning up the codebase, refactors like this should not be part of a project branch or version that maintainers are trying to stabilize and put into maintenance mode. This is under-the-hood stuff, so forcing these changes into 4.x adds no value for users and introduces unnecessary risk for maintainers.

Ideally, I'd prefer we triage the existing issues, get all hands to focus on knocking out the 4.x issues, then turn towards 5.x work (which would include this refactor). Getting through the 4.x work is going to be really hard to do if some folks are trying to resolve issues while a refactor is taking place at the same time.

Hope this doesn't sound discouraging. It definitely isn't meant to be. I think there are some solid changes in here and I'd love to see them merged, just not for v4.

trusktr · 2020-06-16T00:39:09Z

If majority agrees to wait until the next major release to merge this, I can keep merging develop into this branch to keep this updated and ensure tests are passing.

Let's focus on adding tests for each new pull request.

anikethsaha · 2020-06-16T06:23:59Z

We dont have to wait for next major release to merge this. This seems like a non-breaking change and it doesn't effect the behavior of the core.

I guess you dont have to update the branch as it will increase the comments here. We can update it before merge as it gets approvals.

Yea, tests would be awesome.

* develop: docs: removed codefund docs and plugin (#1262) docs: remove bundle size from the home page and documentation (#1257) fix: search can not search the table header (#1256) fix: after setting the background image, the button is obscured (#1234) Fix: fixed onlycover flag in mobile (#1243) fix: Updated docs with instructions for installing specific version (fixes #780) (#1225) fix: Add error handling for missing dependencies (fixes #1210) (#1232) [documdocs: deploy docsify in docker. (#1241) docs: Add embed gist instructions to Embed Files (fixes #932 ) (#1238) chore: add changelog 4.11.4 [build] 4.11.4 feat: added html sanitizer for remote rendering (#1128)

trusktr · 2020-07-04T01:20:18Z

src/core/render/index.js

-    this._renderTo('.cover-main', html);
-    sticky();
-  };
+    _render_updateRender() {


There was previously two functions, both named updateRender, one in the router mixin, one in the render mixin, which is confusing. They were both plain functions before (so basically "private" methods), but now that they are both on this, I renamed them to _render_updateRender and _router_updateRender so they can be differentiated. We can clean it or find better names later...

Apart from this change, nothing major other than converting to classes and all functions are methods.

trusktr · 2020-07-04T01:24:14Z

The changes in this PR aren't too complicated, mostly just syntax changes, and renamed some functions.

It starts to give us better intellisense support in IDEs like VS Code because of the class syntax (powered by TypeScript underneath). I would really like to move forward with it.

I am willing to help resolve any conflicts in other PRs that this may cause.

trusktr · 2021-12-08T00:35:26Z

Closing this in favor of #1685

trusktr commented Jun 7, 2020

View reviewed changes

trusktr marked this pull request as draft June 8, 2020 02:06

vercel bot deployed to Preview June 8, 2020 03:19 View deployment

vercel bot deployed to Preview June 8, 2020 03:22 View deployment

vercel bot deployed to Preview June 8, 2020 03:56 View deployment

vercel bot deployed to Preview June 8, 2020 07:35 View deployment

trusktr added 12 commits June 8, 2020 00:37

convert Docsify to an ES class to start modernizing the code base

4055395

move lifecycle to its own mixin

115a994

move initPlugin into its own pluginMixin, and clean up the mixin appl…

05b569a

…ication site (update buble to fix an unhelpful error message)

move router code into the class

8e982ab

move initRender into the renderMixin class

aa4f97b

move initEvent function into the eventMixin class

71b85be

move fetch code into the fetchMixin class

db08eda

remove unnecessary _init call

8aa4df2

move render methods into renderMixin class

a91c2ce

initialize globals in the index file, so the Docsify.js file only exp…

4e6294a

…orts the Docsify class

add JSDoc comments to start describing each class

511e1c5

update comment

3ec9e97

trusktr force-pushed the docsify-class branch from 368f34b to 3ec9e97 Compare June 8, 2020 07:37

vercel bot deployed to Preview June 8, 2020 07:37 View deployment

trusktr requested review from anikethsaha, sy-records and jhildenbiddle June 9, 2020 21:21

Base automatically changed from allow-config-function to develop June 13, 2020 05:23

trusktr marked this pull request as ready for review June 13, 2020 05:26

vercel bot deployed to Preview June 13, 2020 05:29 View deployment

trusktr requested a review from jamesgeorge007 June 13, 2020 05:30

trusktr requested a review from Koooooo-7 June 13, 2020 05:30

trusktr commented Jun 13, 2020

View reviewed changes

jhildenbiddle reviewed Jun 14, 2020

View reviewed changes

Merge branch 'develop' into docsify-class

e3f5719

vercel bot deployed to Preview June 16, 2020 00:34 View deployment

vercel bot deployed to Preview July 4, 2020 01:14 View deployment

trusktr requested a review from jhildenbiddle July 4, 2020 01:15

trusktr commented Jul 4, 2020

View reviewed changes

Merge branch 'develop' into docsify-class

918574a

vercel bot deployed to Preview July 5, 2020 00:55 View deployment

trusktr closed this Dec 8, 2021

sy-records deleted the docsify-class branch March 8, 2022 04:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert Docsify to an ES class to start modernizing the code base #1206

convert Docsify to an ES class to start modernizing the code base #1206

trusktr commented Jun 7, 2020 •

edited

Loading

vercel bot commented Jun 7, 2020 •

edited

Loading

trusktr commented Jun 7, 2020

trusktr Jun 7, 2020 •

edited

Loading

codesandbox-ci bot commented Jun 13, 2020 •

edited

Loading

trusktr commented Jun 13, 2020

trusktr Jun 13, 2020

trusktr Jun 13, 2020

trusktr Jun 13, 2020

trusktr Jun 13, 2020

trusktr Jun 13, 2020

jhildenbiddle left a comment •

edited

Loading

trusktr commented Jun 16, 2020 •

edited

Loading

anikethsaha commented Jun 16, 2020

trusktr Jul 4, 2020

trusktr Jul 4, 2020

trusktr commented Jul 4, 2020

trusktr commented Dec 8, 2021

convert Docsify to an ES class to start modernizing the code base #1206

convert Docsify to an ES class to start modernizing the code base #1206

Conversation

trusktr commented Jun 7, 2020 • edited Loading

vercel bot commented Jun 7, 2020 • edited Loading

trusktr commented Jun 7, 2020

trusktr Jun 7, 2020 • edited Loading

Choose a reason for hiding this comment

codesandbox-ci bot commented Jun 13, 2020 • edited Loading

trusktr commented Jun 13, 2020

trusktr Jun 13, 2020

Choose a reason for hiding this comment

trusktr Jun 13, 2020

Choose a reason for hiding this comment

trusktr Jun 13, 2020

Choose a reason for hiding this comment

trusktr Jun 13, 2020

Choose a reason for hiding this comment

trusktr Jun 13, 2020

Choose a reason for hiding this comment

jhildenbiddle left a comment • edited Loading

Choose a reason for hiding this comment

trusktr commented Jun 16, 2020 • edited Loading

anikethsaha commented Jun 16, 2020

trusktr Jul 4, 2020

Choose a reason for hiding this comment

trusktr Jul 4, 2020

Choose a reason for hiding this comment

trusktr commented Jul 4, 2020

trusktr commented Dec 8, 2021

trusktr commented Jun 7, 2020 •

edited

Loading

vercel bot commented Jun 7, 2020 •

edited

Loading

trusktr Jun 7, 2020 •

edited

Loading

codesandbox-ci bot commented Jun 13, 2020 •

edited

Loading

jhildenbiddle left a comment •

edited

Loading

trusktr commented Jun 16, 2020 •

edited

Loading