docgen: Optimize README update script #18840
Conversation
aduth
added
[Type] Build Tooling
aduth
requested review from
ajitbohra,
nerrad,
ntwb and
youknowriad
as code owners
|
The performance gains that something like this could yield are great! Thanks for working on this. A couple of things:
Testing instructions for the worst-case scenario# edit the JSON comment of an exported entity in `packages/core-data/src/actions.js`
git add packages/core-data/src/actions.js
git commit -m 'testing'The expected result is that:
|
bin/update-readmes.js
Outdated
| // Each target operates over the same file, so it needs to be processed synchronously, | ||
| // as to make sure the processes don't overwrite each other. | ||
| const { status, stderr } = spawnSync( | ||
| join( __dirname, '..', 'node_modules', '.bin', 'docgen' ).replace( / /g, '\\ ' ), |
There was a problem hiding this comment.
Note: The changes here remove the String#replace fix which was introduced by @MarkMarzeotti in #18253 to improve Windows support.
My reading of the documentation of execa leads me to believe that this should be handled by default in the library, as well as potential other issues we could have with Windows support:
Node has issues when using spawn on Windows:
[...]
- Has problems running commands with spaces
https://github.com/sindresorhus/execa#why
https://github.com/moxystudio/node-cross-spawn#why
Well, my understanding is that we target the current LTS for support. This was documented previously, but apparently was removed as part of the changes in #17004. I'm not really sure why, because it leaves some ambiguity to what we support (commented at #17004 (comment)). Personally, I think we should restore this documented expectation, and not care to support v10 and earlier. |
I wasn't aware of this other script. It seems it's not updated correctly, as per your suspicion. Looking at the code, I'd guess at the very least the keys of "Autogenerated selectors" would need to be updated, else try to adapt similar changes to what was applied here for |
I'm fine with this. However, I wanted to note that, at the moment, node has active two LTS lines and v10 will be LTS until April 2020. Don't know why they do this and I also don't see why we wouldn't upgrade to v12 given that it's easy to install it into any system with a nvm/n kind of setup. I wanted to share anyway to not leave anyone behind. |
Followed-up at #18923 (comment). That's a good observation. In the updated documentation in #18923, I suggest we note this as "latest active LTS", especially when we're already promoting use of |
|
On the topic of I think the use of "Autogenerated actions" and "Autogenerated selectors" could serve as some sort of distinction of what should qualify as data documentation, but there's a few edge cases to consider:
|
oandregal
force-pushed
the
update/fast-update-readmes
branch
from
3958615 to
21963bf
Compare
|
@aduth now that #18820 has landed, I've rebased this in an attempt to ease the burden from you. I hope it's useful, but I still have the old branch locally, in case you rather want it back. This doesn't work as expected as per the instructions at #18840 (comment) (readme package docs aren't updated). We can port the changes to |
|
@nosolosw Thanks for helping out with the refresh! I haven't had a chance yet to review the updates in much detail, but it looks good at a glance. A few things I'd like to do here:
|
aduth
force-pushed
the
update/fast-update-readmes
branch
from
21963bf to
37adca3
Compare
|
Size Change: 0 B Total Size: 860 kB ℹ️ View Unchanged
|
|
I've refreshed this pull request. As I mentioned in previous comments, I was able to absorb all of the functionality of data and package documentation to a single script and eliminate the hard-coded listing of packages. Average run times are ~1.8s for a full run (down from ~6.3s). However, I expect the most common usage will only need to regenerate documentation for a single package (via pre-commit arguments), where the average run time is more in the range of ~0.7s. |
|
I'm reviewing this (and pushed a path fix). |
By removing the package list from the are-readmes-updated.js process we're checking both packages' READMEs and handbook's docs, making are-data-files-unstaged redundant.
|
@sirreal This may be an interesting case to consider for your work in #18942. The changes here seek to opt-in the Can you imagine there to be any compromise? Should we just accept that these scripts won't be type-checked? Would individual top-of-file |
| <!-- END TOKEN(Autogenerated actions) --> | ||
|
|
||
| <!-- END TOKEN(Autogenerated actions|../../../../packages/core-data/src/actions.js) --> |
There was a problem hiding this comment.
Full-transparency: I'm not entirely sure that the extra newline here would be expected. And, if I recall correctly, these were some of the symptoms we saw in the earlier parallelization efforts that ultimately led to their revert. But when I debugged the code flow of the documentation build script, it was always the case that no two docgen processes would run on the same document at the same time, which was the problem we had in the earlier implementation.
There was a problem hiding this comment.
This is something that I've looked and tested specifically by modifying core-data's selectors and actions, whose changes should be reflected in the package's README and the handbook's data-core doc. It's working as expected: different files are processed asynchronously (good for performance), but different tokens within the same file are processed synchronously (required for docgen). So that's 👍
I don't have an answer for the extra line, though. It gives me confidence that it's consistent and doesn't depend on input order: note how the extra line is always added before the end of the second token for all files, while in the master implementation that extra line was always added at the end of the first token. In data-core.md the last item is the actions while in the README.md is the selectors. I've also tested for things like input variance (reversed input data and output is the same):
node ./bin/api-docs/update-api-docs.js packages/core-data/src/selectors.js packages/core-data/src/actions.js
and
node ./bin/api-docs/update-api-docs.js packages/core-data/src/actions.js packages/core-data/src/selectors.js
produce the same results.
aduth
force-pushed
the
update/fast-update-readmes
branch
from
4119f47 to
278ae89
Compare
|
Rebased, since there were failures related to yesterday's issues (#21118). It's end of day for me, so I'll plan to merge this tomorrow assuming everything is in order, and to allow time for the build to pass and potential responses to #18840 (comment) and/or #18840 (comment). I'm not too worried if we just have to remove this from type-checking if it becomes a problem for #18942. |
Thanks for bringing this up.
I don't believe anything in #18942 will be compatible with the changes here. The latest iterations in #18942 continue to include "index" into all the typed projects (not necessarily packages). Scripts in Lines 2 to 13 in 31ea1e7 |
Just to clarify: Do you mean "incompatible"? The remainder of your comment would seem to suggest it'll be possible to include. |
🤦♂ Completely compatible, no incompatibilities 👍 #18942 has been rebased and includes this in its type build/check setup. |
It turns out that it is necessary 😅 See #21467 (comment) . I'll be preparing a pull request shortly. |
Previously: #15679, #15200
This pull request seeks to optimize and refactor the
bin/update-readmes.jsscript. The resulting changes should decrease runtime of this script by 90% or more for typical usage (baseline of 6.65s to 0.6slint-stagedsingle package or 1.275s complete run average).The goal here was largely to reduce the delay in
pre-committasks.Implementation Notes:
Influenced by previous optimizations to the Gutenberg build script in #15230, the approach here uses a
fast-globstream, allowing compilation to begin even before all files are known.Like in #15200, the process is now again asynchronous, but incorporates necessary revisions from #15679 to ensure that tokens within a single file can only be replaced in sequence, not in parallel, to avoid conflict.
It avoids a hard-coded list in favor of reading in the contents of discovered README files to determine whether replacement tokens exist. A "new" syntax was incorporated for files which contain multiple tokens that source from separate files (see "Autogenerated actions", "Autogenerated selectors"). This is really more a convention considered by
update-readmes.js, and doesn't require any revisions to thedocgentool.Finally, since
lint-stagedwill already provide the staged files as arguments to the script, we can use this to filter packages to those where modifications have occurred.Testing Instructions:
Verify that changes to a file covered by
docgenwill update the corresponding README, including those with multiple or custom "Autogenerated" tokens (e.g.core-data).Verify that the behavior of
lint-stagedis not regressed. You can do this by editing a JavaScript file, staging it (git add), then runningnpx lint-staged.