-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
fix(nodes-base): fix and harmonize all primaryDocumentation links #4191
Conversation
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.
Looks good to me.
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.
- URL is case-insensitive so the link will work, but should we be allowing for this casing divergence?
n8n-nodes-base.activecampaigntrigger
vs.n8n-nodes-base.activeCampaignTrigger
The canonical name is camelCased after the prefix. No immediate risk that I can think of, but I would avoid this divergence if we can help it. With the help of a few custom scripts, I was able to locate and fix a couple of 404 links, malformed JSONs, and unnecessary redirects.
Can you add a task to automate this in future? Via CI or an n8n workflow.npx prettier --write nodes/**/*.node.json
Can you update theformat
command to format these files as well?
Our hosting automatically lowercases URLs (this is the background to making the links lowercase) |
That's unfortunate. Please disregard. |
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.
Nice
Perhaps we can also change
return 'https://docs.n8n.io/nodes/' + (nodeType.documentationUrl || nodeType.name) + '?utm_source=n8n_app&utm_medium=node_settings_modal-credential_link&utm_campaign=' + nodeType.name; |
Ahh, great catch! Thanks for pointing this out, I'll check this one. |
63b8b32
const { categories, subcategories, resources: allResources, alias } = require(`${filePath}on`); // .js to .json | ||
|
||
const resources = pick(allResources, ['primaryDocumentation', 'credentialDocumentation']); | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
return { | ||
...(categories && { categories }), | ||
...(subcategories && { subcategories }), | ||
...(resources && { resources }), | ||
...(alias && { alias }), | ||
}; | ||
} |
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.
My thinking is that in the long run the only maintainable way is to get the doc links directly from the node descriptor (codex). I now exposed resources.primaryDocumentation
and resources.credentialDescription
so that the UI can access and use those, instead of having to generate the URLs dynamically from parts.
'' : // Don't show documentation link for community nodes if the URL is not an absolute path | ||
`https://docs.n8n.io/credentials/${type.documentationUrl}/?utm_source=n8n_app&utm_medium=left_nav_menu&utm_campaign=create_new_credentials_modal`; | ||
`${BUILTIN_CREDENTIALS_DOCS_URL}${type.documentationUrl}/?utm_source=n8n_app&utm_medium=left_nav_menu&utm_campaign=create_new_credentials_modal`; |
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.
Parametrized the credential link URL prefix.
// Built-in node documentation available via its codex entry | ||
const primaryDocUrl = nodeType.codex?.resources?.primaryDocumentation?.[0]?.url; | ||
if (primaryDocUrl) { | ||
return primaryDocUrl + utmTags; | ||
} |
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.
All our built-in nodes (with the exception of the StickyNote, I think) have a primary documentation link defined in their descriptor, so let's use that directly where it applies.
resources?: { | ||
credentialDocumentation?: DocumentationLink[]; | ||
primaryDocumentation?: DocumentationLink[]; | ||
}; |
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.
Exposing documentation links as part of a node's CodexData.
@@ -18,7 +18,7 @@ | |||
"dev": "npm run watch", | |||
"build": "tsc && gulp build:icons && gulp build:translations", | |||
"build:translations": "gulp build:translations", | |||
"format": "cd ../.. && node_modules/prettier/bin-prettier.js --write \"packages/nodes-base/**/*.ts\"", | |||
"format": "cd ../.. && node_modules/prettier/bin-prettier.js --write \"packages/nodes-base/**/*.{ts,json}\"", |
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.
Formatting also JSON files now.
@krynble @ivov Thanks for the great input! I refactored how the documentation links are formed, and so far all the ones I clicked would seem to open the correct URL directly. It's possible there's a case I might have missed, but the logic overall seems quite solid. I also updated some more old links that obviously would either get straight up redirected or were just broken. Also created a N8N-4791 for including the helper scripts in the repo, to make it easier to deal with these links in the future. Pointed my most notable changes as line comments. |
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.
LGTM!
Got released with |
…n-io#4191) * fix(nodes-base): fix and harmonize all primaryDocumentation links * feat(workflow, cli): expose documentation links to UI via node codex * fix(editor-ui): link to correct node and credential documentation URLs * config(nodes-base): update 'format' script to also format node descriptor json * chore: fix outdated links to node reference documentation
This is a housekeeping 🏠 🔨 PR to bring all the
primaryDocumentation
links in nodes-base up to date and help @StarfallProjects keep them tidy.With the help of a few custom scripts, I was able to locate and fix a couple of 404 links, malformed JSONs, and unnecessary redirects. The bulk of this change is to crawl all primaryDocumentation links in node descriptor files, and for those that would be redirected (HTTP 301), replace the JSON with their actual target.
Finally, ran
npx prettier --write nodes/**/*.node.json
to pretty-print the files.With this change, all node URLs should take us directly to their correct documentation pages, and we can further clean up some of the manual redirects in the docs.