Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: When alias contains parameters, append ext error#1855

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
sy-records merged 7 commits intodevelopfromfix/1854
Apr 10, 2025

Conversation

@sy-records
Copy link
Member

Summary

As mentioned in#1854, if the URL used by the alias contains parameters, we will add the 'ext' configuration item incorrectly

What kind of change does this PR introduce?

Bugfix

For any code change,

  • Related documentation has been updated if needed
  • Related tests have been updated or tests have been added

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

Related issue, if any:

Fix#1854

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@vercel
Copy link

vercelbot commentedAug 12, 2022•
edited
Loading

The latest updates on your projects. Learn more aboutVercel for Git ↗︎

NameStatusPreviewCommentsUpdated (UTC)
docsify-preview✅ Ready (Inspect)Visit Preview💬Add feedbackApr 9, 2025 10:59am

@codesandbox-ci
Copy link

codesandbox-cibot commentedAug 12, 2022•
edited
Loading

This pull request is automatically built and testable inCodeSandbox.

To see build info of the built libraries, clickhere or the icon next to each commit SHA.

Latest deployment of this branch, based on commit1c5a0cb:

SandboxSource
docsify-templateConfiguration

Copy link
Member

@Koooooo-7Koooooo-7 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

LGTM.
In line 85, it seems a typobug.

Koooooo-7
Koooooo-7 previously approved these changesAug 12, 2022
@sy-recordssy-records requested a review froma teamAugust 13, 2022 02:38
Koooooo-7
Koooooo-7 previously approved these changesAug 13, 2022
:`${path}${ext}`;
:path.indexOf('?')===-1
?`${path}${ext}`
:path;
Copy link
Member

Choose a reason for hiding this comment

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

We need to handle all URLs the same with or without the query, just like regular HTML in a browser.

For example,/path/to/folder/?foo=123 has a query, and we still need to look for/path/to/folder/README.md?foo=123, but this skips anything with a query.

In the end, we need to operate on the path separately, and always send the query with the final result.

SumithChandran reacted with thumbs up emoji

Choose a reason for hiding this comment

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

@trusktr, is there anything that could help push this along? I'm not that familiar with js but I am good with regular expressions. If you can provide a little more context on the issue perhaps I could put a little time into it, as fixing this would open up a lot of doors for those of us needing to federate markdown files from private repos/orgs on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

@vergilcw sorry for late reply. I think the solution should determine the fully qualified URL (f.e.anchor.href = partialUrlString; const fullyQualifiedUrl = anchor.href) and then use anew URL object to get all the pieces and use them correctly.

Copy link
Member

Choose a reason for hiding this comment

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

@sy-records

I think the solution should determine the fully qualified URL (f.e.anchor.href = partialUrlString; const fullyQualifiedUrl = anchor.href) and then use a new URL object to get all the pieces and use them correctly.

Have a chance to try this out?

Copy link
MemberAuthor

@sy-recordssy-recordsFeb 28, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Can you clarify the difference between usingnew URL and the changes in this PR?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

In most cases,path is an Invalid URL, so I don't use it.

image

Copy link
Member

@trusktrtrusktr left a comment

Choose a reason for hiding this comment

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

See comment. Basically this still has other problems. The solution is to handle paths and queries separately

@SumithChandran
Copy link

SumithChandran commentedSep 6, 2022•
edited
Loading

See comment. Basically this still has other problems. The solution is to handle paths and queries separately
Hi,
Any new updates on this issue.
We were working on federating multiple repo's readme.md files to a single documentation page.
Fixing this error would help a lot.

@trusktr
Copy link
Member

See comment. Basically this still has other problems. The solution is to handle paths and queries separately
Hi,
Any new updates on this issue.
We were working on federating multiple repo's readme.md files to a single documentation page.
Fixing this error would help a lot.

@SumithChandran hi, sorry for taking long. Did you find a solution? Icommented on a direction we should look at. We just need to handle all the URL parts correctly, f.e. get all the parts of the URL, do what we need with only the path part, then put everything back together properly.

@SumithChandran
Copy link

See comment. Basically this still has other problems. The solution is to handle paths and queries separately
Hi,
Any new updates on this issue.
We were working on federating multiple repo's readme.md files to a single documentation page.
Fixing this error would help a lot.

@SumithChandran hi, sorry for taking long. Did you find a solution? Icommented on a direction we should look at. We just need to handle all the URL parts correctly, f.e. get all the parts of the URL, do what we need with only the path part, then put everything back together properly.

@trusktr , no I haven't found anything solution.

@zomgbre
Copy link

Any chance this will get done soon? I've actually never committed to a public repo, but this looks pretty straightforward. I'd be willing to try.

yoshikawasan and SumithChandran reacted with heart emoji

@trusktr
Copy link
Member

Any chance this will get done soon? I've actually never committed to a public repo, but this looks pretty straightforward. I'd be willing to try.

@zomgbre willing to take a stab at it? We haven't circled back yet

SumithChandran reacted with thumbs up emoji

Copy link

CopilotAI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@sy-recordssy-records merged commit1a32fb7 intodevelopApr 10, 2025
9 checks passed
@sy-recordssy-records deleted the fix/1854 branchApril 10, 2025 01:13
@sy-recordssy-records mentioned this pull requestMay 5, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

Copilot code reviewCopilotCopilot left review comments

@jhildenbiddlejhildenbiddleAwaiting requested review from jhildenbiddle

@trusktrtrusktrAwaiting requested review from trusktr

@Koooooo-7Koooooo-7Awaiting requested review from Koooooo-7

+1 more reviewer

@vergilcwvergilcwvergilcw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

.md getting appended while using alias

7 participants

@sy-records@SumithChandran@trusktr@zomgbre@vergilcw@Koooooo-7
[8]ページ先頭
©2009-2025 Movatter.jp