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

Support content collections with % in filename #8684

Merged
merged 6 commits into from
Sep 28, 2023
Merged

Conversation

matthewp
Copy link
Contributor

Changes

Testing

  • Test case added to content collections

Docs

N/A, bug fix

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2023

🦋 Changeset detected

Latest commit: 8e8ad71

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 Sep 28, 2023
Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

based on my trust in Node core code (have to fully tried it locally), this lgtm.

packages/astro/src/core/build/static-build.ts Outdated Show resolved Hide resolved
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Have a question about % escaping, but not blocking the PR.

Comment on lines +189 to +199
// Detect if the chunk name has as % sign that is not encoded.
// This is borrowed from Node core: https://github.com/nodejs/node/blob/3838b579e44bf0c2db43171c3ce0da51eb6b05d5/lib/internal/url.js#L1382-L1391
// We do this because you cannot import a module with this character in it.
for(let i = 0; i < name.length; i++) {
if(name[i] === '%') {
const third = name.codePointAt(i + 2)! | 0x20;
if (name[i + 1] !== '2' || third !== 102) {
return `chunks/${name.replace(/%/g, '_percent_')}_[hash].mjs`;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it should be possible to import modules with % but it needs to be escaped before importing. Is it happening inside the Rollup-generated code, or are we importing it wrongly?

Copy link
Contributor Author

@matthewp matthewp Sep 28, 2023

Choose a reason for hiding this comment

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

It's the rollup generated code, it has this exactly line import('../chunks/four%.hash.mjs') and Node cannot handle that. I think this might actually be a Node.js bug; not being able to import valid filename seems like a bug to me. But any event, if we escape it in the filename then everything works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked offline and this is either a Node.js bug or a Rollup bug, so going with the workaround for now. I'll be filing a bug with Node.js to see what they think.

@matthewp matthewp merged commit 824dd46 into main Sep 28, 2023
13 checks passed
@matthewp matthewp deleted the space-in-content branch September 28, 2023 16:20
@astrobot-houston astrobot-houston mentioned this pull request Sep 28, 2023
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Percent in file name causes error in Content Collection
3 participants