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

Add links to npm packages in package.json file view #29344

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
4 changes: 2 additions & 2 deletions web_src/js/features/copycontent.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {clippie} from 'clippie';
import {showTemporaryTooltip} from '../modules/tippy.js';
import {convertImage} from '../utils.js';
import {getFileViewFileText} from '../utils/misc.js';
import {GET} from '../modules/fetch.js';

const {i18n} = window.config;
Expand Down Expand Up @@ -36,8 +37,7 @@ export function initCopyContent() {
btn.classList.remove('is-loading', 'small-loading-icon');
}
} else { // text, read from DOM
const lineEls = document.querySelectorAll('.file-view .lines-code');
content = Array.from(lineEls, (el) => el.textContent).join('');
content = getFileViewFileText();
}

// try copy original first, if that fails and it's an image, convert it to png
Expand Down
3 changes: 2 additions & 1 deletion web_src/js/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {initStopwatch} from './features/stopwatch.js';
import {initFindFileInRepo} from './features/repo-findfile.js';
import {initCommentContent, initMarkupContent} from './markup/content.js';
import {initPdfViewer} from './render/pdf.js';

import {initUserAuthOauth2} from './features/user-auth.js';
import {
initRepoIssueDue,
Expand Down Expand Up @@ -85,6 +84,7 @@ import {initRepoRecentCommits} from './features/recent-commits.js';
import {initRepoDiffCommitBranchesAndTags} from './features/repo-diff-commit.js';
import {initDirAuto} from './modules/dirauto.js';
import {initRepositorySearch} from './features/repo-search.js';
import {initFileView} from './render/code.js';

// Init Gitea's Fomantic settings
initGiteaFomantic();
Expand Down Expand Up @@ -186,4 +186,5 @@ onDomReady(() => {
initRepoDiffView();
initPdfViewer();
initScopedAccessTokenCategories();
initFileView();
});
52 changes: 52 additions & 0 deletions web_src/js/render/code.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import {getFileViewFilePath, getFileViewFileText, createExternalLink} from '../utils/misc.js';
import {basename, isObject} from '../utils.js';

export function initFileView() {
if (document.querySelector('.file-view.code-view')) {
const fileName = basename(getFileViewFilePath());
if (fileName === 'package.json') {
processPackageJson();
}
}
}

function processPackageJson() {
let obj;
try {
obj = JSON.parse(getFileViewFileText());
} catch {
return;
}
if (!isObject(obj)) return;

const packages = new Set();

for (const key of [
'dependencies',
'dependenciesMeta',
'devDependencies',
'optionalDependencies',
'overrides',
'peerDependencies',
'peerDependenciesMeta',
'resolutions',
]) {
for (const packageName of Object.keys(obj?.[key] || {})) {
packages.add(packageName);
}
}

// match chroma NameTag token to detect JSON object keys
for (const el of document.querySelectorAll('.code-inner .nt')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is really fragile, it doesn't seem suitable to be done on frontend.

Copy link
Member Author

@silverwind silverwind Mar 23, 2024

Choose a reason for hiding this comment

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

It's the best frontend can do, and I don't think chroma would ever break it and even if it does, it will be graceful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not right to let frontend do: the JS code only sees the rendered content, and it heavily depends on Chroma behavior.

I am sure there could be a stable backend solution: fully tested and fully controllable.

Copy link
Member Author

@silverwind silverwind Mar 23, 2024

Choose a reason for hiding this comment

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

I'll not be doing that in backend and I think doing it in backend would massively limit future similar "postprocessing".

Doing these file processings in frontend enables to do more that what the backend could do with only HTML rendering. In this case it's possible to do in backend but on other more advanced cases like, frontend side will be required so I prefer to keep it in frontend only.

Take for example GitHub's code view. All the features there like "go to function" and symbol definitions are pure frontend features in React. The backend is just a dumb server of the content and I think that's very preferable.

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 23, 2024

Choose a reason for hiding this comment

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

I'll personally not be doing that in backend and I think doing it in backend would massively limit future similar processings.

I do not think it would limit any future processings. Instead, backend could handle all future processings better than frontend.

For example, if it needs to handle maven XML, in frontend, you need a lot of tricks to to handle the rendered XML-HTML content. But in backend, it sees the clear origin content and could add links clearly.

Copy link
Member Author

@silverwind silverwind Mar 23, 2024

Choose a reason for hiding this comment

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

Have fun implementing a VSCode-like code view in backend 😆. I think these features definitely need to be in frontend and in fact can only be done there.

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 23, 2024

Choose a reason for hiding this comment

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

General talking about "frontend" and "backend" is not clear for this case. Let's abstract the logic:

  • origin content (text) -> highlight (get HTML) -> split to lines (HTML-line slice) -> output to page content

So the question is: add the links between which steps?

My "backend" proposal:

  • origin content (text) -> highlight AND/OR add links (get HTML) -> split to lines (HTML-line slice) -> output to page content

You "frontend" proposal:

  • origin content (text) -> highlight (get HTML) -> split to lines (HTML-line slice) -> output to page content -> use JS to parse the rendered HTML to add links

VSCode: VSCode has a well-designed plugin system, I haven't really looked into it, so I don't know at which step it could add links. My intuition tells me it is very unlikely to do it at the last step (eg: parse the rendered content). If I was a VSCode developer, I would do it like this:

  • origin content (text) -> parse (structured tree) -> plugin processing (add links) -> highlight (get HTML) -> output to editor

Copy link
Member Author

@silverwind silverwind Mar 23, 2024

Choose a reason for hiding this comment

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

Adding only links or any other HTML content can be done in backend but my point is that if the post-processing goes beyong HTML modification, like for example a JSON "block folding" feature, this has do be done in JS and then it's better if all such post-processing code is in frontend (JS) and not split between backend and frontend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if we decide to render code using React or Vue, it will be much easier to migrate the JS code than to migrate it from Go.

const jsonKey = el.textContent.replace(/^"(.*)"$/, '$1');
if (packages.has(jsonKey)) {
const link = createExternalLink({
className: 'suppressed',
textContent: jsonKey,
href: `https://www.npmjs.com/package/${jsonKey}`,
});
el.textContent = '';
el.append('"', link, '"');
}
}
}
22 changes: 22 additions & 0 deletions web_src/js/utils/misc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// returns a file's path from repo root, including a leading slash
export function getFileViewFilePath() {
const pathWithRepo = document.querySelector('.repo-path')?.textContent?.trim();
return `/${pathWithRepo.split('/').filter((_, i) => i !== 0).join('/')}`;
}

// returns a file's text content
export function getFileViewFileText() {
const lineEls = document.querySelectorAll('.file-view .lines-code');
return Array.from(lineEls, (el) => el.textContent).join('');
}

// create a external link with suitable attributes
export function createExternalLink(props = {}) {
const a = document.createElement('a');
a.target = '_blank';
a.rel = 'noopener noreferrer nofollow';
silverwind marked this conversation as resolved.
Show resolved Hide resolved
for (const [key, value] of Object.entries(props)) {
a[key] = value;
}
return a;
}
8 changes: 8 additions & 0 deletions web_src/js/utils/misc.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import {createExternalLink} from './misc.js';

test('createExternalLink', () => {
const link = createExternalLink({href: 'https://example.com', textContent: 'example'});
expect(link.tagName).toEqual('A');
expect(link.href).toEqual('https://example.com/');
expect(link.textContent).toEqual('example');
});