Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: output type definitions#2392

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
trusktr wants to merge5 commits intodevelop
base:develop
Choose a base branch
Loading
fromtype-definitions
Draft

Conversation

trusktr
Copy link
Member

@trusktrtrusktr commentedMar 28, 2024
edited
Loading

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,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

No

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge

Additional info

This outputs types to a newdist/ 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. Wecould 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, asprismjs is in CommonJS format hence native JS modules are not working there yet. To test out the template,

  • runnpm install within the template repo,
  • runnpm link within the docsify repo,
  • runnpm link docsify within the template repo to symlink a local copy of docsify into the template repo
  • openindex.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).

Koooooo-7 reacted with eyes emoji
@vercelVercel
Copy link

vercelbot commentedMar 28, 2024
edited
Loading

The latest updates on your projects. Learn more aboutVercel for Git ↗︎

NameStatusPreviewCommentsUpdated (UTC)
docsify-preview✅ Ready (Inspect)Visit Preview💬Add feedbackApr 2, 2024 7:41am

@sy-recordssy-records changed the titleoutput type definitionschore: output type definitionsMar 28, 2024
@sy-records
Copy link
Member

Do these need to be fixed?

image

@trusktr
Copy link
MemberAuthor

trusktr commentedMar 30, 2024
edited
Loading

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.

sy-records reacted with laugh emoji

sy-records
sy-records previously approved these changesMar 31, 2024
@jhildenbiddle
Copy link
Member

jhildenbiddle commentedMar 31, 2024
edited
Loading

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.

Copy link
Member

@jhildenbiddlejhildenbiddle left a comment
edited
Loading

Choose a reason for hiding this comment

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

As I understand it, this PR is intended to support users who are consuming Docsify as an ESM module, correct?

If so, I don't think we should be distributing TypeScript definitions until end users are able to actually benefit from them. Since Docsify currently can not be consumed as an ESM module, there is no real-world benefit to generating ad.ts file. That may change in the future, but until then these changes only add complexity to the project and stand to confuse users who may wonder why we offer ad.ts file when there is no way to use it.

My concern here is with merging code to support future work that we don't have commitments for or documentation to support. I've implemented similar configurations in my own projects (e.g.mergician) so I am not opposed to the approach used in the PR. I simply prefer to see these changes incorporated as part of a larger "Docsify as ESM module" effort since the benefits of the changes in the PR are only realized when the larger effort is complete.

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

@jhildenbiddlejhildenbiddleMar 31, 2024
edited
Loading

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 ad.ts file. This is after a clean install.

Copy link
MemberAuthor

@trusktrtrusktrMar 31, 2024
edited
Loading

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 exportDocsify and allow it to accept options withnew 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 inthis commit, but my new commit failed something)). However GitHub seems to have some feature that is automatically showing us type errors in theFiles 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
MemberAuthor

@trusktrtrusktrMar 31, 2024
edited
Loading

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).

@trusktrtrusktr marked this pull request as draftMarch 31, 2024 21:46
… package for people to start importing `Docsify` and passing in non-global configs
@Koooooo-7
Copy link
Member

IMO,for current propose to replace$docsify seems not necessary (module-lize).
WDYT?

@trusktr
Copy link
MemberAuthor

trusktr commentedApr 2, 2024
edited
Loading

Let's chat about the concept, let me know what you think of this path.

In thelast commit I marked$docsify as "deprecated", and addedconsole.warn messages. I also exposedsrc/ files inpackage.json``.files, and adjustedexports so that imports of original source modules as well as single-file bundle modules in dist have type definitions visible when imported.

I updated thedemo repository so that each of the following files show how theimport works (with type checking and intellisense) while the current version of Docsify from this PR is symlinked into itsnode_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 theimportmap is not done (it requires handling of prismjs which is UMD format), but it shows that type definitions are working with 4 differentimport methods. People who do have a build would be able to start importingDocsify 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 targetel, 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 --><scriptsrc="path/to/docsify/lib/docsify.min.js"></script><divid="app"><!-- the element gets mounted here --></div>

vs

<!-- modern (HTML file) --><scripttype="module">import'path/to/docsify/dist/docsify.js'</script><docsify-appexecute-scripthomepage="index.md"etc></docsify-app>

or

// modern (JS component)import'path/to/docsify/dist/docsify.js'exportfunctionMyComponent(){return<docsify-appexecute-scripthomepage="index.md"etc></docsify-app>}

etc

Gonna think about this some more...

Koooooo-7 reacted with thumbs up emoji

@trusktr
Copy link
MemberAuthor

trusktr commentedApr 2, 2024
edited
Loading

Theconsole.warn deprecation messages cause some playwrightexpect(consoleMsg).toBeUndefined() tests to fail, but that's easy to fix (should we even be testing that?). The code otherwise still works (back compat).

Copy link
Member

@jhildenbiddlejhildenbiddle left a comment
edited
Loading

Choose a reason for hiding this comment

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

I think this is a good example of why these types of changes should be introduced as either a newissue (better for triage/tracking) ordiscussion (better for higher-level discussions that produce issue(s) to track/triage) instead of a PR. What started off as a PR about generating TypeScript types has evolved into a discussion involving ESM modules, deprecating globals, and componentization. These are all good things to discuss, but the comments section of this PR is not the right place IMHO.

@trusktr -- There's some good stuff in here, so please don't take the above comment negatively. I am onboard with the core concepts of what is being proposed here, but I'd like us to align and execute efficiently since we have limited resources available. For example:

  • We cannot deprecate Docsify globals because we have agreed to keep v5 compatible with v4 with the exception of SSR and legacy browser support. Deprecating globals would therefore have to be targeted for a future release.
  • Wecould offer Docsify as an ESM export for v5 in addition to our current IIFE version, but if we do this we also have to consider if/how we export plugins and themes.
  • I like the idea of providing better IntelliSense for docsify configurations, but the implementation I would use is different than the one in this PR.

Can we create / move to separate issues so can align and triage?

  1. ESM export => New issue
    TypeScript / IntelliSense support can be part of this discussion.
  2. Docsify encapsulation =>#2135
    I've updated this thread with high-level POC. Deprecation of globals can be part of this discussion.

Thanks!

@Koooooo-7
Copy link
Member

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.

@trusktr
Copy link
MemberAuthor

trusktr commentedApr 21, 2024
edited
Loading

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 currentmaxLevel option accepts numbers only. A user will be able to write this:

functionMyReactComponent(){return<docsify-appmax-level={3}/>}

but if they were to write this:

functionMyReactComponent(){return<docsify-appmax-level={"foo"}/>}

they will get a type error in their editor (f.e. VS Code) because"foo" is astring, not anumber.

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!

@trusktr
Copy link
MemberAuthor

trusktr commentedApr 21, 2024
edited
Loading

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 🚀 😄 )

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jhildenbiddlejhildenbiddlejhildenbiddle requested changes

@Koooooo-7Koooooo-7Awaiting requested review from Koooooo-7

@sy-recordssy-recordsAwaiting requested review from sy-records

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@trusktr@sy-records@jhildenbiddle@Koooooo-7

[8]ページ先頭

©2009-2025 Movatter.jp