Generate MD docs for all packages #2033
Conversation
fabioberger
requested review from
BMillman19,
dekz,
fragosti and
steveklebanoff
as code owners
|
fabioberger
force-pushed
the
feature/genMDDocs
branch
from
a48ed4a to
aecceac
Compare
fabioberger
force-pushed
the
feature/genMDDocs
branch
from
eb5bd65 to
7cd1b25
Compare
|
I haven't reviewed all of this, but I wanted to discourage you from checking in generated code. (Deja vu? :P) If I understand correctly, the workflow is set up such that someone could make changes to some TypeScript and forget to also check in changes to the generated markdown. This sounds like a recipe for confusion and headache. The next person who DOES try to update the markdown will have to sift through a bunch of changes that are reflective of prior work, and any reviewer would have to go digging through commit histories in order to make sense of it all. To me the best thing to do would be to not check these in at all, and to persist them as CircleCI Build Artifacts. (See usage of However, I realize there's an argument to be made that persisting as Build Artifacts rather than checked-in code does make it difficult to review changes to the generated markdown. But I would counter that argument by saying that it's always difficult to review a change in rendering (I've always wondered how a web developer reviews changes to the rendering of HTML/JS code in a PR...) and that as a user of a renderer we should just trust that the renderer is tested properly and not doing anything stupid on our behalf. If you must insist that we keep checking in these markdown files, I suggest adding a test that will fail if someone changes the TypeScript source without also updating the generated markdown. This is the approach we plan to take for the |
|
@feuGeneA actually the MD docs will only be generated during the publishing flow and auto-commited to Github along with the updated CHANGELOGS. No one will ever need to review them. We only guarantee that the docs at the publish commit are reflective of the packages at that version, not every intermediate state after each PR. Do you now agree with the decision to commit them? It's nice to have the docs in the commit history for every version through time. |
|
I'm sorry to be a stickler, but I still disagree that they should be checked in. I love the idea of having a canonical/authoritative place for generated docs, and of having a versioning system that explicitly associates them with particular releases, but I don't think the source tree is the right place for that. These are artifacts, not code. It sounds these markdown files are beyond just "build artifacts", and could accurately be described as "release artifacts." Could we leverage the GitHub Releases .zip files as the authoritative place to find the generated docs associated with a particular version? To be clear, I'm not trying to be a purist, and I want to advocate pragmatism. If what you've got here is the most practical solution, then don't let me be a blocker. (I for one wouldn't know where to start with GitHub Releases and could see myself taking the tech-debt-incurring shortcut of just checking these in.) But I do feel pretty strongly that a repo is for source code, not artifacts (whether build, release, or otherwise), and that if we have a good-enough and not-too-onerous mechanism for a particular type of artifact then we should utilize it, and since we do have a mechanism at our disposal for "release artifacts" then we should at least consider using it. |
|
One other thought: If you took the GitHub Releases .zip file approach, maybe then you could skip the S3 step and have the Docs 2.0 site pull the markdown straight from GitHub? I dunno if that makes sense or not, but I wanted to throw it out there for consideration. |
|
What is the benefit of having the mdx files checked into the source code? Is it a requirement for something else or just a nice to have? They're generated on publish. Then versioned and uploaded to s3. They likely also exist in the published npm module. I agree with Gene that we shouldn't have artifacts in the repository which aren't necessary for a usable module, e.g #2044, generated wrappers. If they're used for some other piece of functionality, could they be retrieved via the npm package which would contain these files? |
|
@feuGeneA @dekz S3 will hold MD docs for all 0x packages, across multiple repos, and not only for monorepo packages. This makes it a more sensible place then the release zip. Secondly, the release zip is a very user-unfriendly way of browsing the docs for a package, it is hard to find, requires a download and unzip to read. The MD doc files within the monorepo is a nice-to-have. It's not used by anything but is more for someone preferring to use Github to explore our packages instead of our website. It's also about keeping the docs for posterity's sake, our site will only show the docs for the last 10 versions of a package. If you want the docs for an older package, you have to browse for it on Github. Packaging the docs into the NPM published code is an alternative approach we could consider but it'll increase the bundle size which is not desirable. We could have people browse our S3 bucket but that won't render the markdown nicely. Although I agree with the premise of the argument, I don't think these should cause anyone any grief given that they are only updated during the publish flow, will never be reviewed by a human and won't cause any merge conflicts. |
|
Packaging the docs into the NPM also makes it available offline where the other options require an internet connection to read the docs. I don't think the size increase is going to be large in an node module, since the docs can be compressed well as it is non binary data. Does the same practice hold for other ecosystems though? I.e in go/python deps? I agree that a .zip file is not a great user experience in getting to the docs. |
|
|
||
| • **exchangeAddress**: *string* | ||
|
|
||
| *Inherited from void* |
There was a problem hiding this comment.
Hmm do we need these Inherited from void lines? They are everywhere, any way to get rid of them?
There was a problem hiding this comment.
If I understand correctly, sounds like docs in GH are guaranteed to be consistent with code at publish commits, but not otherwise? I think this is OK, just want to make sure I haven't made a wrong assumption.
I'm generally in favour of having docs in Github:
- preserve old versions
- minimise additional knowledge and steps needed to get to docs from code, since it's in the same location
- all our docs are text-based, so we can see line-by-line diffs of docs between releases. useful for going back and understanding changes
I think the main argument against putting docs in the same place as source code is that it makes sharing difficult if you have access control on the source code - but we don't. 🤷♂
To me the question is more one of - do we need to commit generated code? And if no, what if we just put a note in the README that says something like "To view documentation locally, run yarn docs:md"? That would be so much simpler to maintain AND access than to store it in yet another location. If you've got the code, you've got the docs.
To be clear, we would still maintain docs for recent versions on our website via the publish workflow. Adding a note like that would just enable people to see old versions and access locally.
|
Thanks Xianny. Yes, your understand is correct. There is one issue with your proposal, and that is that it would require a user wanting to read the docs to clone our repo, run I really appreciate everyone's commitment to avoiding adding generated artifacts to the repo but in this case, there isn't really a way to get an easy end-developer experience otherwise (minus @dekz's approach of bundling them into NPM packages which comes at the cost of increased bundle sizes). |
|
Is increased bundle sizes really an issue? We're talking human-readable text, which gets compressed typically 10:1 or better. (Not saying it's not an issue, just saying, perhaps do a quick measure before deciding it is.) |
How can you be so sure? Would you be up for a simple test to enforce it? Like #2012 . If really never to be reviewed by a human, please tag them as |
quick tldr: during CI runs do a |
Yeah, that's correct. I feel pretty neutral about having generated docs on GH. My suggestion was if we decide not to commit the generated docs, I think it would be easier to prompt users to generate locally from the code they are already looking at, than to have them search for our S3, git release files, etc. I think bundling with npm is the most user-friendly solution but self-generating is easy if we are really really opposed to that. I think adding docs to GH does add some overhead, but I also think being able to see diffs between docs could be useful. |
fabioberger
force-pushed
the
feature/genMDDocs
branch
from
10705c1 to
547e9d9
Compare
fabioberger
force-pushed
the
feature/genMDDocs
branch
from
a6f43f5 to
b0e56fc
Compare
|
Thanks for all the ideas and discussions. Turns out the MD files can get quite large (1MB uncompressed for |
Description
This PR:
reference.mdfor the current version of each package that currently has a doc page.The Docs 2.0 site will then involve pulling down the latest versions from S3, indexing the MD files on Algolia and bundling them into the site for faster loading times.
What is cool about this approach is that even packages that live outside the monorepo can be searchable/renderable on the Docs 2.0 site (e.g., the TS client for Mesh 0xProject/0x-mesh#334).
Testing instructions
Types of changes
Checklist:
[WIP]if necessary.