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: output type definitions #2392

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
/_playwright-report
/_playwright-results
/lib
/dist
/node_modules
/.husky/_

Expand Down
3 changes: 3 additions & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@ lib/
themes/
_playwright-*/
emoji-data.*

# for some reason this is needed in CI, although locally Prettier ignored dist/ already.
dist/
3 changes: 2 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{
"editor.defaultFormatter": "esbenp.prettier-vscode",
"cSpell.words": ["coverpage"]
"cSpell.words": ["coverpage"],
"typescript.tsdk": "node_modules/typescript/lib"
}
21 changes: 18 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,37 @@
"type": "module",
"// The 'main' and 'unpkg' fields will remain as legacy for backwards compatibility for now. We will add deprectaion warnings to these outputs to give people time to see the warnings in their apps in a non-breaking way, and eventually we can remove the legacy stuff.": "",
"main": "lib/docsify.js",
"types": "dist/core/Docsify.d.ts",
"unpkg": "lib/docsify.min.js",
"// We're using the 'exports' field as an override of the 'main' field to provide the new ESM setup. Once we remove legacy 'main', we will remove the 'exports' field and have a simple ESM setup with only a 'main' field.": "",
"// These exports require moduleResolution:NodeNext to be enabled in the consumer.": "",
"// TODO native ESM (in browsers) does not work yet because prismjs is not ESM and Docsify source imports it as CommonJS": "",
"exports": {
".": "./src/Docsify.js",
"./*": "./*"
"./*": "./*",
".": {
"types": "./dist/core/Docsify.d.ts",
"default": "./src/core/Docsify.js"
}
},
"files": [
"lib",
"themes"
],
"// Using 'typesVersions' here is the only way we could figure out how to get types working for imports of any subpath without any of the problems other approaches have when not using modeResolution:NodeNext (listed in https://stackoverflow.com/questions/77856692/how-to-publish-plain-jsjsdoc-library-for-typescript-consumers)": "",
"typesVersions": {
"*": {
"src/*": [
"dist/*"
]
}
},
"scripts": {
"build:cover": "node build/cover.js",
"build:css:min": "node build/mincss.js",
"build:css": "mkdirp lib/themes && node build/css -o lib/themes",
"build:emoji": "node ./build/emoji.js",
"build:js": "cross-env NODE_ENV=production node build/build.js",
"build:js": "npm run build:types && cross-env NODE_ENV=production node build/build.js",
"build:types": "tsc -p tsconfig.json || true",
"build:test": "npm run build && npm test",
"build": "rimraf lib themes && run-s build:js build:css build:css:min build:cover build:emoji",
"dev": "run-p serve:dev watch:*",
Expand Down
25 changes: 25 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"compilerOptions": {
"allowJs": true,
"checkJs": true,

"module": "NodeNext",
"moduleResolution": "NodeNext",
"target": "ESNext",
"lib": ["DOM", "ESNext"],
"declaration": true,
"emitDeclarationOnly": true,
Copy link
Member

@jhildenbiddle jhildenbiddle Mar 31, 2024

Choose a reason for hiding this comment

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

This PR currently compiles every .js file in /src/core and /src/plugins to a corresponding .ts file /dist. It is not emitting a d.ts file. This is after a clean install.

Copy link
Member Author

@trusktr trusktr Mar 31, 2024

Choose a reason for hiding this comment

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

The screenshot shows .d.ts declaration files, which is the expected result. That's what makes the types available in the demo:

https://github.com/docsifyjs/docsify-template-plain-js-type-checked/blob/main/index.js

however the demo has docsify locally symlinked, which includes the src code. The lib otherwise doesn't export anything from the index file.

I converted the PR into a draft. Perhaps first we should export Docsify and allow it to accept options with new Docsify({...}) and deprecate global $docsify as a way to move towards a componentization, non-globals, and ESM, and then having the type defs will make a lot more sense.

fails our tests and automated checks

Our tests didn't fail (the PR went green (EDIT: it was green in this commit, but my new commit failed something)). However GitHub seems to have some feature that is automatically showing us type errors in the Files changed tab now (?), but this is not failing our own checks. I recommend we disable it if that's possible, or just ignore it, then fix errors as we go rather than refactoring code all in one PR.

Copy link
Member Author

@trusktr trusktr Mar 31, 2024

Choose a reason for hiding this comment

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

Ok here's what I'll do:

  • I'll make the deprecation note for $docsify in the same PR
  • and expose the new non-global API (new Docsify, along with type support as above).

Let me know if you prefer the change to be separate (it won't be a big additional change, you'll be able to take a look at the specific commit).

"resolveJsonModule": true,
"outDir": "dist/"
},
"include": ["src/**/*.js"],
"exclude": [
// "./**/*.test.js",
// "test/",
// "jest.config.js",
// "middleware.js",
// "playwright.config.js",
// "server.configs.js",
// "server.js"
]
}
Loading