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

fix(markdoc & mdx): Proxy crimes #10278

Merged
merged 2 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/giant-spies-type.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@astrojs/markdoc": patch
"astro": patch
---

Fixes original images sometimes being kept / deleted when they shouldn't in both MDX and Markdoc
1 change: 1 addition & 0 deletions packages/astro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
"dev": "astro-scripts dev --copy-wasm --prebuild \"src/runtime/server/astro-island.ts\" --prebuild \"src/runtime/client/{idle,load,media,only,visible}.ts\" \"src/**/*.{ts,js}\"",
"postbuild": "astro-scripts copy \"src/**/*.astro\" && astro-scripts copy \"src/**/*.wasm\"",
"test": "pnpm run test:node",
"test:match": "pnpm run test:node --match",
Copy link
Member Author

@Princesseuh Princesseuh Feb 29, 2024

Choose a reason for hiding this comment

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

node:test's match is unfortunately terrible, it takes multiple minutes on my computer since it runs through every file still. (also, it outputs every single test with "skipped because it doesn't match", terrible) However, the command in the root repo was broken, so this fixes that.

"test:e2e": "playwright test",
"test:e2e:match": "playwright test -g",
"test:node": "astro-scripts test \"test/**/*.test.js\""
Expand Down
8 changes: 7 additions & 1 deletion packages/astro/src/assets/build/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,13 @@ export async function generateImagesForPath(
!globalThis.astroAsset.referencedImages?.has(transformsAndPath.originalSrcPath)
) {
try {
await fs.promises.unlink(getFullImagePath(originalFilePath, env));
if (transformsAndPath.originalSrcPath) {
env.logger.debug(
'assets',
`Deleting ${originalFilePath} as it's not referenced outside of image processing.`
);
await fs.promises.unlink(getFullImagePath(originalFilePath, env));
}
} catch (e) {
/* No-op, it's okay if we fail to delete one of the file, we're not too picky. */
}
Expand Down
6 changes: 5 additions & 1 deletion packages/astro/src/assets/utils/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ export function getProxyCode(options: ImageMetadata, isSSR: boolean): string {
if (name === 'fsPath') {
return ${stringifiedFSPath};
}
${!isSSR ? `globalThis.astroAsset.referencedImages.add(${stringifiedFSPath});` : ''}
${
!isSSR
? `if (target[name] !== undefined) globalThis.astroAsset.referencedImages.add(${stringifiedFSPath});`
: ''
}
return target[name];
}
})
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/core/logger/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export type LoggerLabel =
| 'preferences'
| 'redirects'
| 'toolbar'
| 'assets'
// SKIP_FORMAT: A special label that tells the logger not to apply any formatting.
// Useful for messages that are already formatted, like the server start message.
| 'SKIP_FORMAT';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import mdx from '@astrojs/mdx';
import markdoc from '@astrojs/markdoc';
import { defineConfig } from 'astro/config';

export default defineConfig({
integrations: [mdx(), markdoc()],
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*"
"astro": "workspace:*",
"@astrojs/mdx": "workspace:*",
"@astrojs/markdoc": "workspace:*"
},
"scripts": {
"dev": "astro dev"
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
title: "Markdoc"
---

There is an image below

![A penguin](../../assets/markdocStillExists.jpg)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
title: "MDX"
---

There is an image below

![A penguin](../../assets/mdxDontExist.jpg)
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { defineCollection, z } from 'astro:content';

const blog = defineCollection({
type: 'content',
schema: z.object({
title: z.string(),
})
});

export const collections = {
blog: blog,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
import type { GetStaticPaths } from "astro";
import { getCollection } from "astro:content";

export const getStaticPaths = (async () => {
const blog = await getCollection("blog");
return blog.map((post) => ({
params: {
slug: post.slug,
},
props: {
post
}
}));
}) satisfies GetStaticPaths;

const { post } = Astro.props;

const { Content } = await post.render();
---

<Content />
12 changes: 11 additions & 1 deletion packages/astro/test/image-deletion.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { before, describe, it } from 'node:test';
import { testImageService } from './test-image-service.js';
import { loadFixture } from './test-utils.js';

describe('astro:assets - delete images that are unused', () => {
describe('astro:assets - delete images that are unused zzz', () => {
/** @type {import('./test-utils.js').Fixture} */
let fixture;

Expand Down Expand Up @@ -33,5 +33,15 @@ describe('astro:assets - delete images that are unused', () => {
const imagesUsedElsewhere = await fixture.glob('_astro/url.*.*');
assert.equal(imagesUsedElsewhere.length, 2);
});

it('should delete MDX images only used for optimization', async () => {
const imagesOnlyOptimized = await fixture.glob('_astro/mdxDontExist.*.*');
assert.equal(imagesOnlyOptimized.length, 1);
});

it('should always keep Markdoc images', async () => {
const imagesUsedElsewhere = await fixture.glob('_astro/markdocStillExists.*.*');
assert.equal(imagesUsedElsewhere.length, 2);
});
});
});
14 changes: 13 additions & 1 deletion packages/integrations/markdoc/src/content-entry-type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,19 @@ async function emitOptimizedImages(
ctx.pluginContext.meta.watchMode,
ctx.pluginContext.emitFile
);
node.attributes[attributeName] = src;

const fsPath = resolved.id;

if (src) {
// We cannot track images in Markdoc, Markdoc rendering always strips out the proxy. As such, we'll always
// assume that the image is referenced elsewhere, to be on safer side.
if (ctx.astroConfig.output === 'static') {
if (globalThis.astroAsset.referencedImages)
globalThis.astroAsset.referencedImages.add(fsPath);
}

node.attributes[attributeName] = { ...src, fsPath };
}
} else {
throw new MarkdocError({
message: `Could not resolve image ${JSON.stringify(
Expand Down
6 changes: 6 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading