fix: download links cannot be refreshed through js · Pull Request #5105 · nodejs/nodejs.org

ghost · 2023-03-12T06:42:21Z

【Problem】

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

ghost · 2023-03-12T06:42:21Z

【Problem】

#5102.

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

ghost · 2023-03-12T06:42:21Z

【Problem】

#5102.

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

ghost · 2023-03-12T06:42:21Z

【Problem】

#5102.

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

ghost · 2023-03-12T06:42:21Z

【Problem】

#5102.

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

ghost · 2023-03-12T06:42:21Z

【Problem】

#5102.

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

ghost · 2023-03-12T06:42:21Z

【Problem】

#5102.

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

ghost · 2023-03-12T06:42:21Z

【Problem】

#5102.

【Reason】

As far as I see, when using 'querySelectorAll', it will first search for the whole page and return you the result directly, sometimes the link name of the element is changed but NOT saved in the storage of querySelector DOM itself.

So A better solution is to search for the element after the DOM frame is loaded without loading the DOM contents...ect.

For more：

Problem: Reason: As far as I see, when using 'querySelectorAll' will store the whole page and return you the result directly, so the page should be fully-loaded. Sometimes there's problems when using "<Script...beforeInactive>" and I change it to "defer async" so as to speed up the loading of js files as well as to make sure the whole page is loaded, we can change the link instead of partially loading.

ghost · 2023-03-12T06:52:45Z

PS：Another way is to change the querySelector to getElementById(), which will find the real element again and again, no matter whether the page is stored or not (Live in searching). But now it seems to change the loading way of js gets me also fine.

ovflowd · 2023-03-12T10:15:12Z

 <Main />
 <NextScript />
- <Script strategy="beforeInteractive" src="/static/js/legacyMain.js" />
+ <script src="/static/js/legacyMain.js" defer async />


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

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？

Not really, it's more like the following:
The next/link component takes an href argument, which is pretty much what our JavaScript legacy script does, replace the URL
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.
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.

ovflowd · 2023-03-12T10:17:35Z

With all due respect, I don't think your PR is fixing anything.

ovflowd · 2023-03-12T12:33:59Z

@MaledongGit, my concerns here are the following:

You seem to be doing many unrelated changes without explaining why.
The description of the PR has nothing to do with the changes nor explains why these changes
You should know that changing the script to async defer doesn't solve any issue; you're deferring the script's load to be the last JavaScript module to be loaded. This can cause many complications, as we're invoking the script "start" function on the Component mount of' Theme'. By deferring its load, the script may not be available when React attempts to call its start method. The next/script with beforeInitiative type ensures this is the first script to load immediately after Next.js's Framework is loaded (before the Application gets loaded).
The bug mentioned in Nodejs download page is acting weird #5102 has nothing to do with the changes of this PR; please read fix: windows download incorrectly selects 32bit on 64bit machine #5025 for context.
The screenshot attached to Nodejs download page is acting weird #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Please, keep these points in consideration. I'm inclined to close this PR, as it doesn't solve any issues, but I want to hear you out, with your points, on why you changed the props of an unrelated component and the label-for part of the Component.

ghost · 2023-03-13T01:08:02Z

why you changed the props of an unrelated component and the label-for part of the Component.

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

ghost · 2023-03-13T01:09:26Z

The screenshot attached to #5102 has an "error" on the console. That is not an error. next/link tries to prefetch routes, but we should not use next/link for external routes.

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "<Link>", however some of them work properly with console errors, should I change them or just fix this?

ghost · 2023-03-13T01:17:19Z

In the end, I found it seems we're now having another fix for bugs like this, so I'd close mine.
Thanks!

ovflowd · 2023-03-13T09:12:41Z

Do you mean "labels["download-for"] ? Ah yes, this is a typo error and it should be this instead of "labels["download"] in i18n. I'm now inverstigating in it and found the problem, just fix it together.

Yeah, I wonder why you made the change? 🤔

ovflowd · 2023-03-13T09:12:56Z

Ah yes, I also found it later, can I help to change them?

I found there're many external routes in "", however some of them work properly with console errors, should I change them or just fix this?

Sure, but I made a PR already hours hours ago hehe

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

MaledongGit added 2 commits March 12, 2023 14:37

Merge branch 'main' into fixJsLoad
30dc9b8

ovflowd reviewed Mar 12, 2023
View reviewed changes

Merge branch 'main' into fixJsLoad
4eb103e

ghost closed this Mar 13, 2023

ghost deleted the fixJsLoad branch March 13, 2023 01:17

This pull request was closed.

Uh oh!

Conversation

ghost commented Mar 12, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovflowd commented Mar 12, 2023

Uh oh!

ovflowd commented Mar 12, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 13, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 13, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 13, 2023

Uh oh!

ovflowd commented Mar 13, 2023

Uh oh!

ovflowd commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ghost commented Mar 12, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovflowd commented Mar 12, 2023

Uh oh!

ovflowd commented Mar 12, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 13, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 13, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 13, 2023

Uh oh!

ovflowd commented Mar 13, 2023

Uh oh!

ovflowd commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ghost commented Mar 12, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ghost commented Mar 12, 2023•
edited by ghost
Loading

ovflowd commented Mar 12, 2023•
edited
Loading

ghost commented Mar 13, 2023•
edited by ghost
Loading

ghost commented Mar 13, 2023•
edited by ghost
Loading

ghost commented Mar 12, 2023•
edited by ghost
Loading

ovflowd commented Mar 12, 2023•
edited
Loading

ghost commented Mar 13, 2023•
edited by ghost
Loading

ghost commented Mar 13, 2023•
edited by ghost
Loading

ghost commented Mar 12, 2023•
edited by ghost
Loading

ovflowd commented Mar 12, 2023•
edited
Loading

ghost commented Mar 13, 2023•
edited by ghost
Loading

ghost commented Mar 13, 2023•
edited by ghost
Loading

ghost commented Mar 12, 2023•
edited by ghost
Loading