-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 configs to be functions #1189
Conversation
…put the Docsify instance
…hat configs can be functions
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/pliopfhjf |
Gotta check what's going on with the failed e2e test. |
Running |
Ooooh, is it because of older Node for one run of the test perhaps? I'll get back to this tomorrow. |
Looks like I should re-do my commits to use the |
I tried switching to Node v10 locally, ran |
yes, refer angular commit convention. I will add commitlint soon to guide with the commit message. |
Worth checking out semantic-release too. But that would be some work to set up. I feel like we need to clean things up, add tests, etc, first. |
yea, this will need changes in the publish script. |
I just noticed something else! All tests pass (at least for me locally, not sure why it fails on CI yet), however, the I will fix this... |
Ok, now that I could debug it (PR #1192), I found out that adding the Let me see here... |
…ld globals in favor of a single global DOCSIFY, and add tests for this
…e-new-single-global update src/core/index.js to export all global APIs, deprecate old globals…
Add build error handling
Nice. All checks have passed. What I could do though, is remove the config function stuff, and just make this a pull requests that adds the test updates, then make a new minimal PR just for the config function. Let me know if that's what I should do. EDIT: The PR is not that big anyways. Turning on the |
@jhildenbiddle @sy-records @Koooooo-7 Can I get one more review from one of you on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned from it. 🚀
@trusktr feel free to fix the merge conflict and merge this. |
Yay. Ok tonight or tomorrow I'll fix the conflict and merge. |
* develop: chore: update auto format config
8f3f263
I fixed the conflict with package-lock.json, and also I opened #1216 which should prevent conflicts from package-lock. Also the e2e test failed. It is flaky. There's a race condition. I'll take a look soon. Let me re-run the tests... |
Co-authored-by: James George <jamesgeorge998001@gmail.com>
tests passed. Merging! |
Summary
Allows user configs to be functions. A function config is passed an instance of the
vm
, which can be useful for referencing in other parts of the config, like inmarkdown.renderer
hooks.Also adds some unit tests in
docsify.test.js
towindow.$docsify
being a function that returns a config object)What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
The PR fulfills these requirements:
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: