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

feat: make error format configurable #9441

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mertcanaltin
Copy link

  • Added ErrorFormatConfig enum to allow configuration of the ErrorFormat used in the minify_sync function.
  • Updated minify_sync to accept an optional error_format parameter, which can be set to "normal", "json", or "color".
  • The error_format parameter defaults to ErrorFormat::Normal if not provided.
  • This change resolves the TODO for making the ErrorFormat configurable, providing greater flexibility in error reporting formats.

can be used in the typeScript field : const result = minify_sync(codeBuffer, optsBuffer, "json");

@mertcanaltin mertcanaltin requested a review from a team as a code owner August 16, 2024 15:10
Copy link

changeset-bot bot commented Aug 16, 2024

⚠️ No Changeset found

Latest commit: 5ff4634

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@CLAassistant
Copy link

CLAassistant commented Aug 16, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Mert Can Altin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

codspeed-hq bot commented Aug 16, 2024

CodSpeed Performance Report

Merging #9441 will degrade performances by 6.34%

Comparing mertcanaltin:dev-001 (7c3980b) with main (50d70d3)

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 175 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main mertcanaltin:dev-001 Change
es/full/bugs-1 553.4 µs 590.9 µs -6.34%
es/full/codegen/es2015 290.1 µs 275.8 µs +5.21%
serialization of serde 2.9 µs 2.8 µs +4.17%

@kdy1 kdy1 added this to the Planned milestone Aug 18, 2024
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Build in CI failed

@mertcanaltin mertcanaltin requested a review from kdy1 August 19, 2024 10:17
@kdy1
Copy link
Member

kdy1 commented Aug 19, 2024

cc @kwonoj Do you have any idea about this?

@@ -16,6 +17,12 @@ use swc_core::{

use crate::{get_compiler, util::try_with};

#[derive(Debug, Deserialize)]
pub enum ErrorFormatConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this option in binding?

Copy link
Author

Choose a reason for hiding this comment

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

The ErrorFormatConfig was introduced to provide flexibility in formatting error outputs, allowing different representations (e.g., JSON) depending on the user's needs. It was included here to streamline error handling directly within the minification process

Copy link
Member

Choose a reason for hiding this comment

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

I mean, why is it stored in bindings/*? I think it's a quite general option and I believe swc_error_reporters is the better place to store this type.

Copy link
Author

Choose a reason for hiding this comment

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

now I understand better thank you I will make an update

pub fn minify_sync(
code: Buffer,
opts: Buffer,
error_format: Option<ErrorFormatConfig>,
Copy link
Member

Choose a reason for hiding this comment

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

Also, why is this an additional parameter, instead of additional field?

Copy link
Author

@mertcanaltin mertcanaltin Aug 21, 2024

Choose a reason for hiding this comment

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

I think reasoning behind using an additional parameter instead of a field was to maintain the separation of concerns. By keeping ErrorFormatConfig as a separate argument, it avoids bloating the existing options structure and allows for easier future extensions or modifications

@kwonoj
Copy link
Member

kwonoj commented Aug 20, 2024

Do you have any idea about this?

Sorry, checking in late. Mind elaborate what you have in mind? the config option itself?

@kdy1
Copy link
Member

kdy1 commented Aug 20, 2024

Yeap, and it being an additional parameter instead of a field.

@kwonoj
Copy link
Member

kwonoj commented Aug 20, 2024

Yes, curious if there's a reason it have to be a 3rd arg instead of extending existing option.

@mertcanaltin
Copy link
Author

Yes, curious if there's a reason it have to be a 3rd arg instead of extending existing option.

decision to introduce it as a third argument rather than extending the existing options was to avoid coupling the error formatting configuration with the core minify options. This modular approach allows users to control error output separately, which can be more intuitive and flexible

@mertcanaltin
Copy link
Author

thank you for your suggestions I sent a commit

@mertcanaltin
Copy link
Author

mertcanaltin commented Aug 21, 2024

I wonder if you have a suggestion about this error @kdy1

I think it's because it's in string type

error[E0308]: mismatched types
  --> binding_core_node/src/minify.rs:85:38
   |
85 |     let task = MinifyTask { c, code, options };
   |                                      ^^^^^^^ expected `String`, found `JsMinifyOptions`

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Please patch

try_with(self.c.cm.clone(), false, ErrorFormat::Normal, |handler| {
let fm = input.to_file(self.c.cm.clone());
self.c.minify(fm, handler, &options)
})
too

bindings/binding_core_node/src/minify.rs Outdated Show resolved Hide resolved
@mertcanaltin mertcanaltin requested a review from kdy1 August 26, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants