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

[content collections] Handle file name spaces and capitalization #5666

Merged
merged 17 commits into from Jan 3, 2023

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Dec 22, 2022

Changes

  • Resolves Content Collections break when file names have spaces #5640
  • Replace incorrect new URL usage with a safer split('?') pattern. This prevents spaces from being resolved to %20.
  • Introduce github-slugger to calculate content slugs. Previously, we simply chopped the extension from the file name. Slugifying each segment should handle capitalization and spaces for use in getStaticPaths.
    • Before: english translations/Using Mdx.mdx -> english translations/Using Mdx
    • After: english translations/Using Mdx.mdx -> english-translations/using-mdx
  • chore: bump to github-slugger 2.0. This migrates from CommonJS to ESM but does not introduce API changes (see release notes).

Testing

  • Add file name with spaces to content-collections fixture
  • Unit test getEntryInfo()

Docs

withastro/docs#2245

@changeset-bot
Copy link

changeset-bot bot commented Dec 22, 2022

🦋 Changeset detected

Latest commit: 0fe19b5

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 the pkg: astro Related to the core `astro` package (scope) label Dec 22, 2022
@bholmesdev bholmesdev changed the title Fix/content spaces [content collections] Handle file name spaces and capitalization Dec 22, 2022
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Dec 22, 2022
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.

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.

@bholmesdev bholmesdev marked this pull request as ready for review December 22, 2022 18:05
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.

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.

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.

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 good to me! Appreciate the tests, awesome work

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.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

code looks good, just need to resolve merge conflicts

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.

@surjithctly
Copy link

surjithctly commented Dec 31, 2022

Does this PR closes issue with spaces in a parent folder name? eg: ./My Works/astro/project

The error was:

[plugin:astro-delayed-asset-plugin] ENOENT: no such file or directory, open 
'/Users/My%20Works/astro/project/src/content/blog/article-name.md'

@bholmesdev
Copy link
Contributor Author

@surjithctly Yes it should! Probably worth an extra unit test for sanity though 👍

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.

@bholmesdev bholmesdev merged commit bf210f7 into main Jan 3, 2023
@bholmesdev bholmesdev deleted the fix/content-spaces branch January 3, 2023 14:45
@astrobot-houston astrobot-houston mentioned this pull request Jan 3, 2023
@surjithctly
Copy link

Sadly, this doesn't fix the space issue in a parent folder. 👎

I think there should be a unit-test as you said to make sure it doesn't have a regression.

@bholmesdev
Copy link
Contributor Author

@surjithctly Well, you're right there 😓 I made the compromise yesterday of merging this fix as-is to ship our minor release, since this PR does solve other existing problems with content collections. Was worth delaying our release for further testing in hindsight.

It also looks like an issue outside of content collections after seeing #5598. We can follow-up there as we investigate a fix!

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.

Content Collections break when file names have spaces
4 participants