Open trusktr opened 6 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 | Apr 2, 2024 7:41am |
Do these need to be fixed?
Not necessary to fix, the code works as-is. Looks like GitHub doing type checking. We can fix type errors as we go along.
Later we can make type definitions for the end users for the options, and for plugins, with nice docs when they hover in their editor.
Do these need to be fixed?
Yes. We should not merge code that fails our tests and automated checks, and we should not merge code assuming someone else will fix the issues later.
Not necessary to fix, the code works as-is.
When we merge code because it "works as-is" despite the fact that it produces errors, fails tests, or fails our automated checks, ever other maintainer is forced to deal with these issues while working locally and while reviewing subsequent PRs.
IMO,for current propose to replace $docsify
seems not necessary (module-lize).
WDYT?
Let's chat about the concept, let me know what you think of this path.
In the last commit I marked $docsify
as "deprecated", and added console.warn
messages. I also exposed src/
files in package.json``.files
, and adjusted exports
so that imports of original source modules as well as single-file bundle modules in dist have type definitions visible when imported.
I updated the demo repository so that each of the following files show how the import
works (with type checking and intellisense) while the current version of Docsify from this PR is symlinked into its node_modules
(to try it, symlink docsify using the same instructions as in the OP):
The template repo is WIP, it can't run because there's no build and the importmap
is not done (it requires handling of prismjs which is UMD format), but it shows that type definitions are working with 4 different import
methods. People who do have a build would be able to start importing Docsify
in apps with webpack, rollup, etc. We can make another template that uses a build.
Another approach would be to go straight for components with solid/react/vue/etc, or even better: custom elements that work in any framework. A "legacy" script could mount a component at the target el
, while component users could import the component/element directly then place it where they want with whatever props/attributes they want.
Hmm, this sounds better.
f.e.
<!-- legacy -->
<script src="path/to/docsify/lib/docsify.min.js"></script>
<div id="app"><!-- the element gets mounted here --></div>
vs
<!-- modern (HTML file) -->
<script type="module">
import 'path/to/docsify/dist/docsify.js'
</script>
<docsify-app execute-script homepage="index.md" etc></docsify-app>
or
// modern (JS component)
import 'path/to/docsify/dist/docsify.js'
export function MyComponent() {
return <docsify-app execute-script homepage="index.md" etc></docsify-app>
}
etc
Gonna think about this some more...
The console.warn
deprecation messages cause some playwright expect(consoleMsg).toBeUndefined()
tests to fail, but that's easy to fix (should we even be testing that?). The code otherwise still works (back compat).
Hi Joe @trusktr , no offense. I understand and respect that you did everything good for necessary changes like always (notification,warning and compatibility).
Based on current @jhildenbiddle mentioned here, I think I may have some misunderstood on the scope.
Is it a planning or something? If it were a planning, thats fine and we could have more discussions on it, I apologize that I declined it directly.
If it were a things you propose to do it now. Sorry that I don't see the clear context/roadmap more on the changes in the PR, so I don't we could get more benefit on the changes now, and it gets more breaking stuff, so I "voted"
the -1
here for the changes.
If I do miss some details plz let me know.
Hey guys, no offense taken at all! I usually propose ideas in the form of code changes. This could be a prototype to help discuss how type checking (without writing TS, only JS with JSDoc comments) can be implemented.
I'm currently experimenting with Astro, and will get a custom element setup with SSR/SSG working in Astro with Lume Element (not Docsify related, just Astro + Lume Element only, to learn about the approach).
One thing that would be nice for when we eventually get Docsify converted into a Custom Element with SSR/SSG working, is to have type checking.
In Astro, for example, all markup is type checked. So for example, it will be possible for a user to write the following in Astro, React JSX, Solid JSX, etc, and they will get type checking. For example, the current maxLevel
option accepts numbers only. A user will be able to write this:
function MyReactComponent() {
return <docsify-app max-level={3} />
}
but if they were to write this:
function MyReactComponent() {
return <docsify-app max-level={"foo"} />
}
they will get a type error in their editor (f.e. VS Code) because "foo"
is a string
, not a number
.
So basically with this PR I'm just starting to get things in place for this future, but it may not be in the ideal final format yet. Let's use this as a discussion point so we can ideate.
In the meantime I'm in Astro land (https://astro.build/chat) doing some research. If you go there 🚀 let me know!
Can we create / move to separate issues so can align and triage?
@jhildenbiddle yeah please feel free to make issues as needed. I have to admit I will not be too active over here while I'm in Astro land. But I'll be back with some info on what I find.
(I'm imagining it as if I'm traveling across the galaxy lol. May as well have some fun 🚀 😄 )
and adjust package.json so that types are picked up in consumer projects
Summary
The code base is still plain JS as before (the build did not change) but this now allows outputting declaration files so that consumers that will start to use ES Modules will get type definitions.
Related issue, if any:
What kind of change does this PR introduce?
Feature (dev experience) Build-related changes (adds a build step that outputs declaration files, but otherwise does not change the build in any other way)
For any code change,
Does this PR introduce a breaking change?
No
Tested in the following browsers:
Additional info
This outputs types to a new
dist/
folder. I figured I'd output here because the intent we have is to output to move all outputs todist/
so I figured I'd get started. We could output types somewhere else liketypes/
, but I figured why do that and then move it later, when we can put it where we know we'll want it.This new (WIP) template shows that type definitions are picked up:
https://github.com/docsifyjs/docsify-template-plain-js-type-checked
Note that the template is not complete yet, as
prismjs
is in CommonJS format hence native JS modules are not working there yet. To test out the template,npm install
within the template repo,npm link
within the docsify repo,npm link docsify
within the template repo to symlink a local copy of docsify into the template repoindex.js
in VS Code and see that running Go To Definition onDocsify
will take you to the type definition, confirming that type definitions are available in consumers. (A consumer with a build step would not have to worry about the native JS module issue).