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

Potentially incorrect links at the bottom of the commonJs doc #57094

Closed
dario-piotrowicz opened this issue Feb 16, 2025 · 6 comments · Fixed by #57098
Closed

Potentially incorrect links at the bottom of the commonJs doc #57094

dario-piotrowicz opened this issue Feb 16, 2025 · 6 comments · Fixed by #57098
Labels
doc Issues and PRs related to the documentations.

Comments

@dario-piotrowicz
Copy link
Contributor

dario-piotrowicz commented Feb 16, 2025

Affected URL(s)

https://nodejs.org/docs/latest/api/modules.html#the-module-object_1, https://nodejs.org/docs/latest/api/modules.html#source-map-v3-support

Description of the problem

There are some links at the bottom of the commonjs doc that seem out of place to me:
Image

In the md file they are prefixed by this comment:

<!-- Anchors to make sure old links find a target -->

which does leads me to believe that these should not be rendered in the page

About the comment

The same comment can be found in other parts of the codebase, the anchors/elements below such comment are indeed not rendered:

node/doc/api/esm.md

Lines 117 to 119 in 1d8593e

<!-- Anchors to make sure old links find a target -->
<i id="esm_package_json_type_field"></i><i id="esm_package_scope_and_file_extensions"></i><i id="esm_input_type_flag"></i>

node/doc/api/esm.md

Lines 142 to 144 in 1d8593e

<!-- Anchors to make sure old links find a target -->
<i id="esm_package_entry_points"></i><i id="esm_main_entry_point_export"></i><i id="esm_subpath_exports"></i><i id="esm_package_exports_fallbacks"></i><i id="esm_exports_sugar"></i><i id="esm_conditional_exports"></i><i id="esm_nested_conditions"></i><i id="esm_self_referencing_a_package_using_its_name"></i><i id="esm_internal_package_imports"></i><i id="esm_dual_commonjs_es_module_packages"></i><i id="esm_dual_package_hazard"></i><i id="esm_writing_dual_packages_while_avoiding_or_minimizing_hazards"></i><i id="esm_approach_1_use_an_es_module_wrapper"></i><i id="esm_approach_2_isolate_state"></i>

node/doc/api/module.md

Lines 1617 to 1619 in 1d8593e

<!-- Anchors to make sure old links find a target -->
<a id="module_module_findsourcemap_path_error"></a>

cc @aduh95

@dario-piotrowicz dario-piotrowicz added the doc Issues and PRs related to the documentations. label Feb 16, 2025
@dario-piotrowicz
Copy link
Contributor Author

PS: additionally the new SourceMap(payload) and sourceMap.findEntry(lineNumber, columnNumber) links are actually broken

@aduh95
Copy link
Contributor

aduh95 commented Feb 16, 2025

In the md file they are prefixed by [a] comment which does leads me to believe that these should not be rendered in the page

The goal is definitely for those to be rendered, it would be quite useless to keep the anchors without showing the link to the actual docs.
The goal is for folks who would follow a link dating from the time those docs where on this page, they get a link to the new emplacement in the docs – of course, it'd be more effective if said link was not broken, I've opened #57098 to address this

@dario-piotrowicz
Copy link
Contributor Author

it would be quite useless to keep the anchors without showing the link to the actual docs.

I thought the point of these were to have anchors with outdated id in this doc, so that old urls could still point to valid locations (for renamed sections etc...), so not something to be displayed, but just there so that urls with fragments would indeed find valid locations to scroll to

I might be misunderstanding what these are for

@aduh95
Copy link
Contributor

aduh95 commented Feb 16, 2025

I thought the point of these were to have anchors with outdated id in this doc, so that old urls could still point to valid locations […] so that urls with fragments would indeed find valid locations to scroll to

That's exactly it

so not something to be displayed

Well you're wrong on that point, it would be rather useless to have the user scroll to the end of the document if there's nothing there.

@dario-piotrowicz
Copy link
Contributor Author

Well you're wrong on that point, it would be rather useless to have the user scroll to the end of the document if there's nothing there.

I see, sorry that's totally my bad, I've just only now noticed that they were linking to a different page! (I didn't notice that the current page is modules but that they link to module 🤦)

Ok then makes sense to me (I thought they were links to a location on the same page!)

Sorry for the dumb mistake 🙂 🙇

aduh95 added a commit to aduh95/node that referenced this issue Feb 18, 2025
PR-URL: nodejs#57098
Fixes: nodejs#57094
Refs: nodejs#48461
Refs: nodejs#47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit to aduh95/node that referenced this issue Feb 18, 2025
PR-URL: nodejs#57098
Fixes: nodejs#57094
Refs: nodejs#48461
Refs: nodejs#47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit to aduh95/node that referenced this issue Feb 18, 2025
PR-URL: nodejs#57098
Fixes: nodejs#57094
Refs: nodejs#48461
Refs: nodejs#47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this issue Feb 20, 2025
PR-URL: #57098
Backport-PR-URL: #57129
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@aduh95 @dario-piotrowicz and others