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

formatOptions is not applied on base image #15

Open
jonisapp opened this issue May 3, 2023 · 5 comments
Open

formatOptions is not applied on base image #15

jonisapp opened this issue May 3, 2023 · 5 comments

Comments

@jonisapp
Copy link

jonisapp commented May 3, 2023

When uploading image files, the formatOptions are only applied to "imageSizes" and not to the base image.

import { S3UploadCollectionConfig } from 'payload-s3-upload';

const TextImages: S3UploadCollectionConfig = {
	slug: 'texts_images',
	labels: { singular: 'Texte - image', plural: 'Textes - images' },
	access: {
		read: () => true,
	},
	admin: {
		group: 'Fichiers',
		disableDuplicate: true,
	},
	upload: {
		staticURL: '/texts/images',
		disableLocalStorage: true,
		s3: {
			bucket: 'lepitre-public',
			prefix: 'texts/images',
			commandInput: {
				ACL: 'public-read',
			},
		},
		imageSizes: [
			{
				name: 'thumbnail',
				width: 200,
				height: undefined,
				formatOptions: {
					format: 'webp',
					options: {
						quality: 70,
					},
				},
			},
		],
		formatOptions: {
			format: 'webp',
			options: {
				quality: 70,
			},
		},
		adminThumbnail: 'thumbnail',
		mimeTypes: ['image/*'],
	},
	hooks: {
		afterRead: [
			async ({ doc }) => {
				doc.url = `https://sos-ch-gva-2.exo.io/lepitre-public/texts/images/${doc.filename}`;

				Object.keys(doc.sizes).forEach(
					(k) =>
						(doc.sizes[
							k
						].url = `https://sos-ch-gva-2.exo.io/lepitre-public/texts/images/${doc.sizes[k].filename}`)
				);
			},
		],
	},
};

export default TextImages;
@jeanbmar jeanbmar added bug Something isn't working help wanted Extra attention is needed and removed bug Something isn't working help wanted Extra attention is needed labels May 16, 2023
@jeanbmar
Copy link
Owner

Hello, is it something that works properly in Payload?
We don't do anything specific with this option in the plugin, as we just process the file buffer passed by Payload.
I've checked the official plugin and they do the same.

@JosefBredereck
Copy link

Not quite correct:

Payload does adjust the base image a bit.

https://github.com/payloadcms/payload/blob/0d8d7f358d390184f6f888d77858b4a145e94214/src/uploads/generateFileData.ts#L88-L105

Moreover, you can pass formatOptions for each size, which could lead to a mimeType change of the size image. Therefore, it is not recommended to use the mime type of the original:

files.push({
buffer,
filename,
mimeType: data.mimeType,
});

The result is not included in req.payloadUploadSizes, it contains only the separate sizes.
https://github.com/payloadcms/payload/blob/0d8d7f358d390184f6f888d77858b4a145e94214/src/uploads/generateFileData.ts#L156

Maybe the data object could give a general idea about the content contained in the edited file.

const getFilesToUpload: CollectionBeforeChangeHook = ({
data,
req,
}): File[] => {

@jeanbmar
Copy link
Owner

I was talking about the cloud storage plugin of Payload, sorry if it was misleading.

The problem I see is that the fileBuffer created at https://github.com/payloadcms/payload/blob/0d8d7f358d390184f6f888d77858b4a145e94214/src/uploads/generateFileData.ts#L115 isn't attached to the object that is returned later.

As a result plugins can't access the transformed image buffer. It's a bug from Payload imo, I see no reason not to pass the transformed buffer to the plugin chain.
For sure we don't want to duplicate the image transformation logic in a s3 storage plugin.

@jeanbmar
Copy link
Owner

jeanbmar commented Jun 18, 2023

I'm wrong Payload does pass the transformed image buffer downstream.
I don't see where the issue is located.
Payload does all the transformations first, then the plugin simply retrieves the buffers passed downstream and upload objects to S3.

What should be done differently in here?:

const reqFile = req.files?.file ?? req.file ?? null;
if (reqFile == null) return [];
const files: File[] = [
{
filename: data.filename,
mimeType: data.mimeType,
buffer: reqFile.data,
},
];
if (data.mimeType?.includes('image') && data.sizes != null) {
Object.entries<FileData>(data.sizes).forEach(([key, sizeData]) => {
const buffer = req.payloadUploadSizes[key];
const { filename } = sizeData;
if (buffer != null || filename != null) {
files.push({
buffer,
filename,
mimeType: data.mimeType,
});
}
});
}
return files;

We just get data passed by Payload after transformations are applied.

@JosefBredereck
Copy link

I think payload does not expose the result file buffer directly, it is included in someway, but not correctly referenced.

Line 33 ignores the mime type of the created file size completely

I think req.files?.file ?? req.file contains the original file. And the transformed is included in data.files.

But if data is just the upload config, then req.files should be type of array. If req contains this result object:
{ data: newData, files: filesToSave }

I'm not quite sure what payload is exactly giving to the plugins. I'm just trying to give an initial idea of what could be the cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants