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

Remove dependency on path-to-regexp #11983

Merged

Conversation

uwej711
Copy link
Contributor

@uwej711 uwej711 commented Sep 13, 2024

Changes

  • Remove dependency path-to-regexp
  • Replace route placeholders directly in generate function

Testing

All existing test still run and code is already covered.

Docs

No change to docs necessary as this is only an internal implementation change.

Copy link

changeset-bot bot commented Sep 13, 2024

🦋 Changeset detected

Latest commit: f40fede

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

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release labels Sep 13, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@uwej711 uwej711 force-pushed the feature/remove-dependency-on-path-to-regexp branch from 87146a7 to 45f136b Compare September 13, 2024 09:08
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 13, 2024
@uwej711 uwej711 changed the base branch from next to main September 13, 2024 09:09
@uwej711 uwej711 force-pushed the feature/remove-dependency-on-path-to-regexp branch from 45f136b to f40fede Compare September 13, 2024 10:15
@github-actions github-actions bot removed the pkg: integration Related to any renderer integration (scope) label Sep 13, 2024
@uwej711
Copy link
Contributor Author

uwej711 commented Sep 13, 2024

Resolves #11956

@delucis delucis linked an issue Sep 13, 2024 that may be closed by this pull request
1 task
@delucis delucis dismissed github-actions[bot]’s stale review September 13, 2024 10:46

This is now a patch — no block needed, thank you robot!

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

These changes look good to me and nice to see all the tests passing and we get to be once dependency lighter.

Thank you @uwej711!

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you for contributing this!

@delucis delucis merged commit 633eeaa into withastro:main Sep 13, 2024
14 checks passed
@Princesseuh Princesseuh mentioned this pull request Sep 13, 2024
@astrobot-houston astrobot-houston mentioned this pull request Sep 12, 2024
matthewp added a commit that referenced this pull request Sep 13, 2024
matthewp added a commit that referenced this pull request Sep 13, 2024
* Revert "Remove dependency on path-to-regexp (#11983)"

This reverts commit 633eeaa.

* Add test for regression

* Add a changeset

* Pin path-to-regexp
@matthewp
Copy link
Contributor

This unfortunately had to be reverted due to a regression. This PR has the revert: #11993 There's now a test so if you want to take another look at it maybe it's not too hard to fix.

@uwej711
Copy link
Contributor Author

uwej711 commented Sep 13, 2024

Thanks for the information. This will happen when the value for a rest parameter is not supplied. In that case an extra slash or a trailing slash is added to the generated path. It should work when we remove double and trailing slashes from the generated path. I will give it a try over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro relies on vulnerable path-to-regexp
4 participants