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(codegen): remove non_exhaustive on Config #8113

Closed

Conversation

dsherret
Copy link
Contributor

@dsherret dsherret commented Oct 13, 2023

It is incredibly useful to create Config using a struct expression in order to be notified about any config changes and to set the config exactly how the consumer would like.

let config = crate::swc::codegen::Config {
    minify: false,
    ascii_only: false,
    omit_last_semi: false,
    target: ES_VERSION,
    // with a struct expression, I get a compiler error when upgrading swc and
    // can decide how I want this functionality to behave. Additionally, I'm forced
    // to explicitly document the config behaviour I want.
    emit_assert_for_import_attributes: ,
}

@kdy1 kdy1 requested review from kwonoj and kdy1 October 13, 2023 04:53
@kdy1
Copy link
Member

kdy1 commented Oct 13, 2023

@kwonoj Any thoughts about this? It was a noticable maintenance burden

@@ -5,7 +5,6 @@ use swc_ecma_ast::EsVersion;
#[derive(Debug, Clone, Copy)]
#[cfg_attr(feature = "serde-impl", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde-impl", serde(rename_all = "camelCase"))]
#[non_exhaustive]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kdy1 I'm a little confused about the maintenance burden of not having this. It seems like this struct rarely ever changes?

image

Of those, it seems like it's changed one time per year, so it just requires a minor version bump on update, especially if all downstream swc crates do not use the struct expression.

Overall, it's not a big deal to have this, I just personally think it's a much better API for consumers to use the struct expression, which is why I opened this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I made fewer changes to avoid breaking changes.

pub fn preamble(&mut self, s: &str) -> Result {

I think this should go into the Config struct, but I didn't want a breaking change.

Copy link
Member

@kdy1 kdy1 Oct 13, 2023

Choose a reason for hiding this comment

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

There were many similar situations IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I thought there's a script in swc that automatically updates all the versions on a breaking change. I'll close this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Versions in Cargo.toml are automatic, but actually the problem is that I don't have control over mdxjs-rs and I have to wait for a long time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah I just re-read your other comment.

@dsherret dsherret closed this Oct 13, 2023
@dsherret dsherret deleted the fix_remove_non_exhaustive branch October 13, 2023 07:01
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants