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

Refactor formatter logic #27

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Conversation

sairus2k
Copy link

@sairus2k sairus2k commented Jan 17, 2025

Fixes #19, #26

Description

Replaced Prettier and Biome API calls with tinyexec process execution.

Type of change

Please mark relevant options with an x in the brackets.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Algorithm update - updates algorithm documentation/questions/answers etc.
  • Other (please describe):

This might be a breaking change. SVG will no longer be formatted if Biome is set as a formatter.

I found and fixed some bugs while working on the PR. Here's what I did:

  1. The biome config in the test project was ignored for some reason. This caused changes in the test-apps/remix-vite/public/icons/types.ts file.
  2. Prettier config in non-JSON format was ignored.

How Has This Been Tested?

  • Integration tests
  • Unit tests
  • Manual tests
  • No tests required

Reviewer checklist

Mark everything that needs to be checked before merging the PR.

  • Check if the UI is working as expected and is satisfactory
  • Check if the code is well documented
  • Check if the behavior is what is expected
  • Check if the code is well tested
  • Check if the code is readable and well formatted
  • Additional checks (document below if any)

Screenshots (if appropriate):

Questions (if appropriate):

Replaced Prettier and Biome api calls with `tinyexec` for formatting and streamlined formatter execution. Deprecate `pathToFormatterConfig` option.
stdinStream.push(fileContent);
stdinStream.push(null);

const { process } = exec(formatter, options, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

finally got the time to go through this, this all looks good but I have a question about this specific bit here, does this require you to have the formatter globally installed? or can it execute biome/prettier from the node_modules, eg I have a project using biome, I don't have to do npm i -g biome for this to work?

Copy link
Author

Choose a reason for hiding this comment

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

Correct, it runs the formatter from node_modules, no need for global install, and I doubt that global formatter will work here.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome, that's what I wanted to hear

@AlemTuzlak AlemTuzlak merged commit 8de76e0 into forge-42:main Feb 4, 2025
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.

Make Prettier and Biome optional peer dependencies
2 participants