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.

Sign up for GitHub

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

Restructure links extracts to keep original links #1055

Merged
merged 5 commits into from
Sep 5, 2022
Merged

Restructure links extracts to keep original links #1055

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d32caaf
Restructure links extracts to keep original links
dontcallmedom Sep 1, 2022
84dda39
Associate link with proper version when matching version-less series
dontcallmedom Sep 2, 2022
90e1183
Separate annotation of links with spec destination into postProcessing
dontcallmedom Sep 2, 2022
f3e68e0
Simplify canonicalizeUrl code
dontcallmedom Sep 5, 2022
6565feb
Use spec-mode for annotate-links post processing
dontcallmedom Sep 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Use spec-mode for annotate-links post processing 
As suggested in #1055 (comment)
  • Loading branch information
@dontcallmedom
dontcallmedom committed Sep 5, 2022
commit 6565feb05144cecae400eff00cd67156f4b88b98
2 changes: 1 addition & 1 deletion src/lib/specs-crawler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ async function saveSpecResults(spec, settings) {
async function crawlList(speclist, crawlOptions) {
// Make a shallow copy of crawl options object since we're going
// to modify properties in place
crawlOptions = Object.assign({}, crawlOptions);
crawlOptions = Object.assign({speclist}, crawlOptions);
Copy link
Member

Choose a reason for hiding this comment

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

We've never been very clean on separating inputs and outputs. Unless I missed a copy somewhere, we directly store the results of the crawl within the spec objects in speclist, at least temporarily while the results get saved to disk. This means speclist may also contain a few expanded crawl results (or all of them when useCrawl is set or when the user does not specify an output folder), up to >50MB of data.

Here, we're asking Puppeteer to send the list to a separate Chrome process, so Puppeteer will need to serialize/deserialize the list each time. That works but that seems like a waste of processing time and memory.

I think we should rather make a deeper copy here and create a new list of spec objects that only contain core metadata about each spec (and that won't be touched by the crawl). That will still send >500KB of data but that should be fine.

I can live with it, but it could perhaps be argued that the extraction code should remain atomic in the sense that its results should only depend on information about the spec and on the spec's contents (extraction code is not the easiest bit to debug since it runs in a headless Chrome instance, so better keep the logic simple). An alternative would be to split the extraction of links into two parts:

  1. The extraction of the raw links in Chrome through extract-links.mjs without attempting to link them back to specs.
  2. A post-processing module that associates extracted links with specs and runs in the Node.js process. Post-processing is a good place to run logic that needs to look at other specs.

Copy link
Member Author

Choose a reason for hiding this comment

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

That approach makes more sense indeed; I've updated the PR to that effect


// Expand list of processing modules to use if not already done
crawlOptions.modules = expandBrowserModules(crawlOptions.modules);
Expand Down
43 changes: 14 additions & 29 deletions src/postprocessing/annotate-links.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,37 +22,22 @@ const needsSaving = {};

module.exports = {
dependsOn: ['links'],
input: 'crawl',
input: 'spec',
property: 'links',

run: function(crawl, options) {
crawl.results.forEach(s => {
for (let link of Object.keys(s.links || {})) {
// Annotate with the spec to which the page belong if we can find one
const specUrl = canonicalizeUrl(link);
let matchingSpec = crawl.results.find(s => s?.release?.url === specUrl || s?.nightly?.url === specUrl || (s?.series?.currentSpecification === s?.shortname && (s?.series?.nightlyUrl === specUrl || s?.series?.releaseUrl === specUrl)) || s?.nightly?.pages?.includes(specUrl) || s?.release?.pages?.includes(specUrl));
if (matchingSpec) {
needsSaving[s.shortname] = true;
s.links[link].specShortname = matchingSpec.shortname;
}
run: function(spec, {speclist}) {
if (!speclist || !speclist.length) {
console.error("No spec list passed as input, cannot annotate links in post-processing");
return spec;
}
for (let link of Object.keys(spec.links || {})) {
// Annotate with the spec to which the page belong if we can find one
const specUrl = canonicalizeUrl(link);
let matchingSpec = speclist.find(s => s?.release?.url === specUrl || s?.nightly?.url === specUrl || (s?.series?.currentSpecification === s?.shortname && (s?.series?.nightlyUrl === specUrl || s?.series?.releaseUrl === specUrl)) || s?.nightly?.pages?.includes(specUrl) || s?.release?.pages?.includes(specUrl));
if (matchingSpec) {
spec.links[link].specShortname = matchingSpec.shortname;
}
});
return crawl;
},

save: async function({results}, options) {
return Promise.all(Object.values(results).map(async spec => {
const contents = {
spec: {
title: spec.title,
url: spec.crawled
},
links: spec.links
};
const json = JSON.stringify(contents, null, 2);
const folder = path.join(options.output, "links");
const filename = path.join(folder, `${spec.shortname}.json`);
return await fs.promises.writeFile(filename, json);
}));
}
return spec;
}
};