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

Write index routes as index.html with build.format: 'file' #9209

Closed
wants to merge 4 commits into from

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Nov 28, 2023

Changes

Testing

  • Test case added

Docs

Copy link

changeset-bot bot commented Nov 28, 2023

🦋 Changeset detected

Latest commit: a619adc

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) pr: docs A PR that includes documentation for review labels Nov 28, 2023
@bluwy
Copy link
Member

bluwy commented Nov 28, 2023

I think the previous behaviour is expected to me:

  • format: 'file' forces foo/index.html to foo.html
  • format: 'directory' forces foo.html to foo/index.html

The "forcing" style intentionally doesn't respect how you declare your files. With this PR, it also slightly conflicts with our recommendation here:

To prevent inconsistencies with trailing slash behaviour in dev, you can restrict the trailingSlash option to 'always' or 'never' depending on your build format:

  • directory - Set trailingSlash: 'always'
  • file - Set trailingSlash: 'never'

Practically speaking though, I prefer that Astro doesn't force a certain build format style and always respect how the files were declared. This PR seems to make build.format: 'file' that way, but maybe it makes more sense if it's build.format: 'preserve' instead 🤔

let dir;
// If the pathname is '' then this is the root index.html
// If this is an index route, the folder should be the pathname, not the parent
if(pathname === '' || routeData.index) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe we could omit the check for an empty pathname since, in this scenario, it's guaranteed to represent an index route, isn't it?

@matthewp
Copy link
Contributor Author

@bluwy I, good point. I'll move this back into draft for discussion then.

@matthewp matthewp marked this pull request as draft November 28, 2023 16:28
@matthewp
Copy link
Contributor Author

@bluwy what would you think about adding this as format: 'preserve', keeping 'file' as it currently is, and then deprecating 'file' (remove in the next major)?

@bluwy
Copy link
Member

bluwy commented Nov 29, 2023

That makes sense to me 👍 aside from this part:

and then deprecating 'file' (remove in the next major)?

I think it might still be worth keeping it. Currently it seems like there's a symmetry with trailingSlash that:

build.format trailingSlash
file never
directory always
preserve ignore

So if we plan on keeping trailingSlash: 'never', it might make sense to also keep build.format: 'file'. The confusing thing is that we have two options that are now even similar 😅 The last time I tracked this down lead me to #1197 but I don't understand why there's two options.

@matthewp
Copy link
Contributor Author

We're probably going to do this in a minor release as the discussion above about making this a new format means it will not be a breaking change. So deprioritizing, but keeping on my personal board.

Base automatically changed from next to main November 30, 2023 15:03
@matthewp
Copy link
Contributor Author

Closing the PR, but will reopen later with new implementation.

@matthewp matthewp closed this Dec 27, 2023
@bluwy bluwy deleted the build-format-change branch October 8, 2024 06:19
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Index pages appear to be invalid according to web standards (i.e. Apache MultiViews and others)
4 participants