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

fix(techdocsStorageClient): properly construct baseUrls #8365

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

kuangp
Copy link
Member

@kuangp kuangp commented Dec 3, 2021

Signed-off-by: Phil Kuang pkuang@factset.com

Hey, I just made a Pull Request!

Context: mkdocs/mkdocs#2456

We have some docs with the older style of relative links as shown in the linked issue above.
eg. [linking to other page](../foo-bar) which loads /docs/default/Component/my-component/foo-bar (note the missing trailing slash)

Looking into the TechDocs reader, this is inherently supported when attempting to load the raw docs html and inspecting the network requests, it is being fetched correctly since the trailing slash is added appropriately:

async getEntityDocs(entityId: EntityName, path: string): Promise<string> {
const { kind, namespace, name } = entityId;
const storageUrl = await this.getStorageUrl();
const url = `${storageUrl}/${namespace}/${kind}/${name}/${path}`;
const token = await this.identityApi.getIdToken();
const request = await fetch(
`${url.endsWith('/') ? url : `${url}/`}index.html`,
{
headers: token ? { Authorization: `Bearer ${token}` } : {},
},
);

The issue arises when updating links that load additional assets:

const newValue = await techdocsStorageApi.getBaseUrl(
elemAttribute,
entityId,
path,
);

eg. a <link href="../extra.css"> is incorrectly transformed to <link href="<app-origin>/static/docs/default/Component/extra.css" />

This change fixes the URL transformation to ensure a trailing slash so that the asset path is resolved correctly eg. <link href="<app-origin>/static/docs/default/Component/my-component/extra.css" />

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@kuangp kuangp requested review from a team as code owners December 3, 2021 21:21
@changeset-bot
Copy link

changeset-bot bot commented Dec 3, 2021

🦋 Changeset detected

Latest commit: e7cce2b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@backstage/plugin-techdocs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@OrkoHunter OrkoHunter added this to Incoming in TechDocs project board via automation Dec 5, 2021
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If you are the author and the PR has been closed, feel free to re-open the PR and continue the contribution!

@github-actions github-actions bot added the stale label Dec 10, 2021
@kuangp
Copy link
Member Author

kuangp commented Dec 10, 2021

@iamEAP can you take a look at this change when you get a chance please ?

@github-actions github-actions bot removed the stale label Dec 10, 2021
@OrkoHunter
Copy link
Member

Hey @kuangp ! Eric is on vacation as of now - please be patient as someone in @backstage/techdocs-core can take a look! 🙏

Although this looks good to me - @emmaindal thoughts?

Copy link
Member

@emmaindal emmaindal left a comment

Choose a reason for hiding this comment

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

Sorry @kuangp I did look at this last week, just didn't approve 😄 Can you prefix the changeset with techdocs-*

Signed-off-by: Phil Kuang <pkuang@factset.com>
@kuangp
Copy link
Member Author

kuangp commented Dec 13, 2021

thanks @emmaindal ! updated the changeset

@camilaibs camilaibs merged commit ab1a69b into backstage:master Dec 13, 2021
TechDocs project board automation moved this from Incoming to Done ✅ Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants