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

fix: download links cannot be refreshed through js #5105

Closed
wants to merge 3 commits into from
Closed

fix: download links cannot be refreshed through js #5105

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca7d3fb
fix: download links cannot be refreshed through js
SEWeiTung Mar 12, 2023
30dc9b8
Merge branch 'main' into fixJsLoad
SEWeiTung Mar 12, 2023
4eb103e
Merge branch 'main' into fixJsLoad
SEWeiTung Mar 12, 2023
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
8 changes: 4 additions & 4 deletions layouts/DownloadLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import SecondaryDownloadMatrix from '../components/Downloads/SecondaryDownloadMa
import { useNextraContext } from '../hooks/useNextraContext';
import { useNodeData } from '../hooks/useNodeData';

import type { LegacyDownloadsFrontMatter, NodeVersionData } from '../types';
import type { LegacyDownloadsFrontMatter } from '../types';

const DownloadLayout = ({ children }: PropsWithChildren) => {
const nextraContext = useNextraContext();
const { currentLtsVersion = {} as NodeVersionData } = useNodeData();
const { currentLtsVersion } = useNodeData();

const { downloads } = nextraContext.frontMatter as LegacyDownloadsFrontMatter;

Expand All @@ -24,8 +24,8 @@ const DownloadLayout = ({ children }: PropsWithChildren) => {

{children}

<PrimaryDownloadMatrix {...currentLtsVersion} />
<SecondaryDownloadMatrix {...currentLtsVersion} />
<PrimaryDownloadMatrix {...currentLtsVersion!} />
<SecondaryDownloadMatrix {...currentLtsVersion!} />
</article>
</div>
</BaseLayout>
Expand Down
2 changes: 1 addition & 1 deletion layouts/IndexLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const IndexLayout = ({ children }: PropsWithChildren) => {
<Banner />

<h2 id="home-downloadhead" data-dl-local={labels['download-for']}>
{labels['download']}
{labels['download-for']}
</h2>

<HomeDownloadButton {...currentLtsVersion!} />
Expand Down
3 changes: 1 addition & 2 deletions pages/_document.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import Script from 'next/script';
import { Html, Head, Main, NextScript } from 'next/document';
import Link from 'next/link';

Expand All @@ -13,7 +12,7 @@ export default function Document() {
<body>
<Main />
<NextScript />
<Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
<script src="/static/js/legacyMain.js" defer async />
Copy link
Member

Choose a reason for hiding this comment

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

Why removing the Script tag from nextjs? Which kind o tests you made that resulted in this conclusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the customized event handlers' problem for listening after all the other js files of next.js are loaded successfully for querySelectoAll()……I also wonder if it's a better solution？

Copy link
Member

Choose a reason for hiding this comment

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

Not really, it's more like the following:

  1. The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
  2. But the thing is that next/link also sets up "click" events and other kinds of events. These events are managed by a React component, meaning if there's a re-render or whatever the internal logic is, it will ignore our "href" attribute.
  3. Not to mention, "click" event has precedence over "href". So basically, us replacing the "href" probably has no effect at that point in time. Note that I am just making a wild guess here.

But the actual solution is for now to not use next/link, we probably also should not use it for external links, as it was primarily made for internal links.

<Link rel="me" href="https://social.lfx.dev/@nodejs" />
</body>
</Html>
Expand Down