-
Notifications
You must be signed in to change notification settings - Fork 349
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
Add initial docs for React module #605
Conversation
Also add bsdoc as a devDep to allow generating docs locally and/or in CI/CD. Commands to generate docs: npx bsdoc build React npx bsdoc support-files # One-time setup but must be second step open docs/React/React/index.html
type callback('input, 'output) = 'input => 'output; | ||
|
||
/* TODO: should we even bother to bind the first two? The whole point of |
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.
@rickyvetter (and/or others): does it make sense to use useCallback
and useMemo
hooks without dependencies?
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.
Ummm. Yeah should probably remove these. Good callout.
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.
Really like this. Just curious about long term strategy for documentation here. How do these generated docs play with the docusaurus site and content on the rescript site? I think the code can land whenever but I'd like to understand publishing strat before we do anything that search engines perpetuate forever.
@@ -1,15 +1,33 @@ | |||
[@text "React bindings for ReasonML"]; |
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.
What's the difference between this and /** React bindings for ReasonML */;
?
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.
Due to a bug in Reason 3.6 (which Jordan fixed recently), floating doc comments /** */
i.e. ones which are not attached to any structure item, are incorrectly printed as [@doc "..."]
instead of the correct attribute which is [@text "..."]
. So we need to directly use [@text "..."]
for these.
@@ -1,15 +1,33 @@ | |||
[@text "React bindings for ReasonML"]; | |||
|
|||
[@text "{1 Elements}"]; |
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.
Can we make it more obvious what this does? I'd prefer something like [@docs.text "foo"];
. Not a blocker but it confused me and wanted to point it out.
The @ over % is so that this doesn't need to exist at compile time, right?
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.
Would [@docs.text "foo"]
work? I always assumed that without a namespace the default one would be [@ocaml.text]
, which is what it's short for.
The @
is required I believe because that's what OCamldoc/odoc require.
type callback('input, 'output) = 'input => 'output; | ||
|
||
/* TODO: should we even bother to bind the first two? The whole point of |
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.
Ummm. Yeah should probably remove these. Good callout.
/** A React element; the equivalent of elements that get created in | ||
JavaScript with [React.createElement]. */ |
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.
Excited to get these in editors as well.
@rickyvetter thanks for the review! To your question about how to integrate with Docusaurus sites, that may be a bit tricky. I'm not an expert on Docusaurus but when I researched it, it seemed to require all pages to be input in Markdown format. The documentation generated by bsdoc (really, odoc) is HTML format (with one CSS and one JS support file). If we can find a way to just serve the contents of the generated docs (it will be by default in a Another thing I should mention: right now, odoc understands and can output OCaml and ReasonML syntaxes. If you want to generate docs in ReScript syntax, either the syntax support will need to be added to odoc (@ryyppy did this for Reason syntax), or some other workaround will be needed. |
Can we split this up in two consecutive PRs that 1) add the missing inline docs and 2) add automatic docs generation? Merging (1) would probably be a nobrainer. For 2) i believe there's still some work left to be able to render the docs in a meaningful way for ReScript developers, and I'd rather not expose them to the default odoc format tbh. |
@ryyppy sure, I'll remove bsdoc from this PR. Nowadays I actually think it's better to do the doc generation part in a build step in CI and not have a dependency on bsdoc. The odoc/ocamldoc format is the only way I know right now to get doc formatting, so not sure what that would be replaced with... |
Done in React.rei |
Also add bsdoc as a devDep to allow generating docs locally and/or in
CI/CD. Commands to generate docs:
Fix #559