Skip to content
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

tools/doc/versions.js should cache version data #32512

Closed
rvagg opened this issue Mar 26, 2020 · 4 comments
Closed

tools/doc/versions.js should cache version data #32512

rvagg opened this issue Mar 26, 2020 · 4 comments
Labels
good first issue Issues that are suitable for first-time contributors.

Comments

@rvagg
Copy link
Member

rvagg commented Mar 26, 2020

make doc forces a version fetch which fetches the CHANGELOG.md from this repo. This is done for each of the doc files that requires the versions list (because they are run one at a time). We should have caching in place so that the first one saves the data somewhere and subsequent runs just pick that it up. For speed, and also to save our CI hammering GitHub each time this is run.

Ref: #32511

@MylesBorins MylesBorins added the good first issue Issues that are suitable for first-time contributors. label Mar 27, 2020
@hassaanp
Copy link
Contributor

hassaanp commented Mar 27, 2020

Hey @rvagg,

Do you think that the following flow makes sense:

  1. Initialize and store a timestamp value at the start of the run
  2. Fetch latest CHANGELOG.md and store it in .cache/timestamp-CHANGELOG.md
  3. Check if local .cache/timestamp-CHANGELOG.md exists locally and use that otherwise repeat step 2

@rvagg
Copy link
Member Author

rvagg commented Mar 27, 2020

Maybe simpler than that, this will be in a clean repo (in the git clean -fdx sense) and this file can be added to .gitignore so it could be in the top level, .master_CHANGELOG.md or something like that. If it exists, use it, if it doesn't exist, fetch it. I don't think there's a good reason to do any timestamping on this.

Unless someone thinks otherwise?

Using https://nodejs.org/download/release/index.json is another option, which would be easy to process, but I guess the reason we use CHANGELOG.md is that we always have a local copy if this needs to be done in offline mode. We just don't have a local master copy unless we're actually running on master, which will lead to a truncated version list.

@hassaanp
Copy link
Contributor

Acknowledged.

Here is my approach in versions.js

    ...
    const masterChangelog = path.join(srcRoot, '.master-CHANGELOG.md');
    if (kNoInternet) {
      changelog = readFileSync(file, { encoding: 'utf8' });
    } else if (existsSync(masterChangelog)) {
      changelog = readFileSync(masterChangelog, { encoding: 'utf8' });
    } else {
      try {
        changelog = await getUrl(url);
        createMasterChangelog(masterChangelog, changelog);
      }
    ...

the createMasterChangelog uses writeFileSync to create the .master-CHANGELOG.md

What do you think

hassaanp added a commit to hassaanp/node that referenced this issue Mar 27, 2020
…ons.js

When running `make doc` the CHANGELOG.md file is pulled everytime for each
of the doc file that requires the versions list. This commit introduces a
new file called `.master-CHANGELOG.md` which will be created once so that
subsequent calls for the version are pulled locally instead of from the repo.
Since this file is not part of the codebase proper, it is appended in the
.gitignore file.

References Issue nodejs#32512
@richardlau
Copy link
Member

richardlau commented Mar 27, 2020

I've had some half finished code to address this for awhile (started since #31849 (review)). Tidied up today (using up work vacation today so had time) and submitted as #32518.

MylesBorins pushed a commit that referenced this issue Mar 31, 2020
Refactor the logic for working out the previous versions of Node.js for
the API documentation so that the parsing (including the potential https
get) happens at most once per build (as opposed to the current once per
generated API doc).

Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: #32518
Fixes: #32512
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 31, 2020
Refactor the logic for working out the previous versions of Node.js for
the API documentation so that the parsing (including the potential https
get) happens at most once per build (as opposed to the current once per
generated API doc).

Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: #32518
Fixes: #32512
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
codebytere pushed a commit that referenced this issue Mar 31, 2020
Refactor the logic for working out the previous versions of Node.js for
the API documentation so that the parsing (including the potential https
get) happens at most once per build (as opposed to the current once per
generated API doc).

Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: #32518
Fixes: #32512
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
richardlau added a commit to richardlau/node-1 that referenced this issue Apr 6, 2020
Refactor the logic for working out the previous versions of Node.js for
the API documentation so that the parsing (including the potential https
get) happens at most once per build (as opposed to the current once per
generated API doc).

Signed-off-by: Richard Lau <riclau@uk.ibm.com>

Backport-PR-URL: nodejs#32642
PR-URL: nodejs#32518
Fixes: nodejs#32512
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 6, 2020
Refactor the logic for working out the previous versions of Node.js for
the API documentation so that the parsing (including the potential https
get) happens at most once per build (as opposed to the current once per
generated API doc).

Signed-off-by: Richard Lau <riclau@uk.ibm.com>

Backport-PR-URL: #32642
PR-URL: #32518
Fixes: #32512
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors.
Projects
None yet
4 participants