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

Integrate Prettier formatting in SVG icon generator script #419

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

MoSattler
Copy link
Contributor

@MoSattler MoSattler commented Aug 31, 2023

Problem:

The current SVG icon generator script produces code that doesn't adhere to the Prettier code style guidelines. As a result, users often face unexpected diffs when running the build:icons or format.

Example:

One such inconsistency is that the type IconName is generated using double quotes (") and a semicolon - while Prettier enforces single quotes (') and no semicolon.

Proposed Solution:

To ensure consistent formatting, I've updated the writeIfChanged function in the icon generator script. After writing the new content to a file, Prettier is invoked to format the file according to the project's Prettier configuration.

async function writeIfChanged(filepath: string, newContent: string) {
	const currentContent = await fsExtra
		.readFile(filepath, 'utf8')
		.catch(() => '')
	if (currentContent === newContent) return false
	await fsExtra.writeFile(filepath, newContent, 'utf8')
+	await $`prettier --write ${filepath}`
	return true
}

Considerations:

I realize that running Prettier for every file generation might seem excessive. However, this approach guarantees that the generated code will always comply with the project's Prettier configuration, preventing any unwanted diffs. An alternative approach is to adjust the code generation logic to produce Prettier-compliant code directly, but that would require manual adjustments if the Prettier configuration changes in the future.

I'm open to feedback and suggestions. If there's a preference to adjust the code generation logic instead, I'm happy to make that change.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks!

@kentcdodds kentcdodds merged commit c2529e8 into epicweb-dev:main Aug 31, 2023
5 checks passed
@sergio-codel-io
Copy link

Hi @MoSattler @kentcdodds , this change is giving me an error when trying to start a new epic-stack project, the error is as follows:

Error: Command failed with exit code 2: prettier --write /home/sergio/Dev/test-workspace/epic/app/components/ui/icons/sprite.svg
[error] No parser could be inferred for file "/home/sergio/Dev/test-workspace/epic/app/components/ui/icons/sprite.svg".
at makeError (file:///home/sergio/Dev/test-workspace/epic/node_modules/execa/lib/error.js:60:11)
at handlePromise (file:///home/sergio/Dev/test-workspace/epic/node_modules/execa/index.js:124:26)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at writeIfChanged (/home/sergio/Dev/test-workspace/epic/other/build-icons.ts:151:2)
at generateIconFiles (/home/sergio/Dev/test-workspace/epic/other/build-icons.ts:54:24)
at (/home/sergio/Dev/test-workspace/epic/other/build-icons.ts:25:2) {
shortMessage: 'Command failed with exit code 2: prettier --write /home/sergio/Dev/test-workspace/epic/app/components/ui/icons/sprite.svg',
command: 'prettier --write /home/sergio/Dev/test-workspace/epic/app/components/ui/icons/sprite.svg',
escapedCommand: 'prettier --write "/home/sergio/Dev/test-workspace/epic/app/components/ui/icons/sprite.svg"',
exitCode: 2,
signal: undefined,
signalDescription: undefined,
stdout: '',
stderr: '[error] No parser could be inferred for file "/home/sergio/Dev/test-workspace/epic/app/components/ui/icons/sprite.svg".',
cwd: '/home/sergio/Dev/test-workspace/epic',
failed: true,
timedOut: false,
isCanceled: false,
killed: false

the error goes away when I comment out the added lines in file build-icons.ts

@MoSattler
Copy link
Contributor Author

I was able to reproduce the reported issue.

I've submitted a PR addressing this problem. You can find the fix here: #421.

Thank you for bringing this to our attention!

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

Successfully merging this pull request may close these issues.

3 participants