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.

Sign up for GitHub

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

Jump to bottom

Fix for #9673 #9680

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Fix for #9673 #9680

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab2535d
Fix for #9673
loucyx Jan 12, 2024
098fa35
🦋 add changeset file
loucyx Jan 12, 2024
17872ed
Update breezy-plants-smoke.md
loucyx Jan 12, 2024
84f8d68
⚡️ simplified normalizeConfigPath
loucyx Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/breezy-plants-smoke.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

🏷️ update type generation for content config.
loucyx marked this conversation as resolved.
Show resolved Hide resolved
37 changes: 26 additions & 11 deletions packages/astro/src/content/types-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,28 @@ function invalidateVirtualMod(viteServer: ViteDevServer) {
viteServer.moduleGraph.invalidateModule(virtualMod);
}

/**
* Takes a configPath and returns a normalized relative version:
* - If is not relative, it adds `./` to the beginning.
* - If it ends with `.ts`, it removes the extension.
* - It adds `.js` to the end.
* - It stringifies the result (adds `""` around it).
* @param from Config path from path.
* @param to Config path to path.
* @returns Normalized config path.
*/
function normalizeConfigPath(from: string, to: string) {
const normalizedConfigPath = path.relative(from, to);
ematipico marked this conversation as resolved.
Show resolved Hide resolved

return JSON.stringify(
`${isRelativePath(normalizedConfigPath) ? '' : './'}${
normalizedConfigPath.endsWith('.ts')
Copy link
Member

Choose a reason for hiding this comment

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

There are also .d.ts files. Are they covered by this check on purpose?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK it should not be .d.ts? So is it worth handling this case?

Copy link
Contributor Author

@loucyx loucyx Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we never get .d.ts and is always .ts then we can update this to replace .ts with .js simplifying the code a bit:

function normalizeConfigPath(from: string, to: string) {
	const normalizedConfigPath = path.relative(from, to);

	return `"${
		isRelativePath(normalizedConfigPath) ? "" : "./"
	}${normalizedConfigPath.replace(/\.ts$/, ".js")}"`;
}

Copy link
Member

Choose a reason for hiding this comment

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

If we never get .d.ts

That's what I not sure about (I am not familiar with this part of the code). So maybe keeping the code as is better.

Copy link
Member

Choose a reason for hiding this comment

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

cc @bholmesdev

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this looks like the loader for src/content/config.ts correct? If so, we only support .ts and .js. .d.ts shouldn't be possible 👍

? normalizedConfigPath.replace(/\.ts$/, '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this always resolves to a file that ends with .ts or .js then we can simplify normalizeConfigPath a bit by moving that .js at the end of the returned string here, so the line ends up being:

? normalizedConfigPath.replace(/\.ts$/, ".js")

Copy link
Contributor

@bholmesdev bholmesdev Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fair point. I believe .mjs would be possible, and this change would silently leave the .mjs extension without changing it. I'd prefer that to be the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll update the PR with the simplified code, then. Thank you!

: normalizedConfigPath
}.js`
);
}

async function writeContentFiles({
fs,
contentPaths,
Expand Down Expand Up @@ -444,18 +466,11 @@ async function writeContentFiles({
fs.mkdirSync(contentPaths.cacheDir, { recursive: true });
}

let configPathRelativeToCacheDir = normalizePath(
path.relative(contentPaths.cacheDir.pathname, contentPaths.config.url.pathname)
const configPathRelativeToCacheDir = normalizeConfigPath(
contentPaths.cacheDir.pathname,
contentPaths.config.url.pathname
);

if (!isRelativePath(configPathRelativeToCacheDir))
configPathRelativeToCacheDir = './' + configPathRelativeToCacheDir;

// Remove `.ts` from import path
if (configPathRelativeToCacheDir.endsWith('.ts')) {
configPathRelativeToCacheDir = configPathRelativeToCacheDir.replace(/\.ts$/, '');
}

for (const contentEntryType of contentEntryTypes) {
if (contentEntryType.contentModuleTypes) {
typeTemplateContent = contentEntryType.contentModuleTypes + '\n' + typeTemplateContent;
Expand All @@ -465,7 +480,7 @@ async function writeContentFiles({
typeTemplateContent = typeTemplateContent.replace('// @@DATA_ENTRY_MAP@@', dataTypesStr);
typeTemplateContent = typeTemplateContent.replace(
"'@@CONTENT_CONFIG_TYPE@@'",
contentConfig ? `typeof import(${JSON.stringify(configPathRelativeToCacheDir)})` : 'never'
contentConfig ? `typeof import(${configPathRelativeToCacheDir})` : 'never'
);

await fs.promises.writeFile(
Expand Down