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

Implements build.format: 'preserve' #9764

Merged
merged 11 commits into from
Jan 31, 2024
Merged

Implements build.format: 'preserve' #9764

merged 11 commits into from
Jan 31, 2024

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Jan 22, 2024

Changes

Testing

New test cases added

Docs

  • Types are updated in this PR
  • No other docs on this (I think)

Copy link

changeset-bot bot commented Jan 22, 2024

🦋 Changeset detected

Latest commit: 7b929e2

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

@matthewp matthewp added this to the 4.3 milestone Jan 22, 2024
@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Jan 22, 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.

@matthewp matthewp marked this pull request as ready for review January 22, 2024 17:43
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.

This looks fine to me!

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Can you add more test cases with:

  • trailingSlash set to always
  • base set to something

@matthewp
Copy link
Contributor Author

@ematipico I added a test for base.

I don't think trailingSlash has any impact on this feature. If you use trailingSlash: 'always' then you essentially have to use page/index.astro and never page.astro, since the latter would 404. build.format is really only a feature of the build and controls how files are written out in SSG mode. I'd be happy to add more tests but I can't think of what I would be testing.

@ematipico
Copy link
Member

@matthewp that's true and I agree with you, although we added a new branching logic inside the function shouldAppendForwardSlash, which is a function that is used in many places of our code base.

So for the sake of code coverage, I'd like to see at least one test that covers this case.

@matthewp
Copy link
Contributor Author

@ematipico The shouldAppendForwardSlash is only used in the build for displaying the pageName. It's used in i18n as well, so should I add an i18n test as well? The build format is only taken into account with trailingSlash: 'ignore'.

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
@firxworx
Copy link

Regarding discussion on trailingSlash note current issue impacting dev server vs. build behaviour: #9674

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you @matthewp ! Happy to merge it

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks @matthewp ! I left a suggestion for each of the changeset and the docs to see if we could make this super explicit about what's going on. Obviously, check that this is correct, but see if my suggestions make sense, or give you ideas for more explicitly laying out the difference (especially between file and preserve, which took the most work to really make clear, I think.)

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
.changeset/tame-flies-confess.md Outdated Show resolved Hide resolved
ematipico and others added 3 commits January 30, 2024 09:17
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@ematipico
Copy link
Member

Hey @sarah11918, I reviewed your suggestions, and they seemed very sound to me. I merged them! Thank you

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just correcting three tiny punctuation/conjunction nits, but approved by docs! 🥳

.changeset/tame-flies-confess.md Outdated Show resolved Hide resolved
.changeset/tame-flies-confess.md Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
@matthewp
Copy link
Contributor Author

Edits look good to me, thanks @sarah11918 !

@ematipico ematipico merged commit fad4f64 into main Jan 31, 2024
14 checks passed
@ematipico ematipico deleted the build-format-preserve branch January 31, 2024 14:39
@astrobot-houston astrobot-houston mentioned this pull request Jan 31, 2024
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) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants