-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(es/typescript): inline enum #3647
Conversation
Does this fix #940? |
5274af9
to
7bda1d7
Compare
Partially. |
|
||
impl Default for InlineEnumConfig { | ||
fn default() -> Self { | ||
Self::All(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdy1
I set "inline all as possible" as default option.
What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good enough to be default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a risk to make it default.
See https://www.typescriptlang.org/play?#code/KYOwrgtgBAglDeAoKsA0UBCiC+jEAo4BDAZyiJAE8BKAbQHIZ6BdKAXigEYBuA4sijVoAGVh06IgA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
use swc_ecma_utils::ident::IdentLike; | ||
use swc_ecma_visit::{as_folder, noop_visit_mut_type, Fold, VisitMut, VisitMutWith}; | ||
|
||
pub type TSEnumLit = Rc<RefCell<AHashMap<Id, AHashMap<JsWord, Lit>>>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to a struct which hides the actual type?
The field can be like
pub(crate) values: Rc<RefCell<AHashMap<Id, AHashMap<JsWord, Lit>>>>
|
||
impl Default for InlineEnumConfig { | ||
fn default() -> Self { | ||
Self::All(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, we have two options I guess.
The first one is adding a field to Assumptions
and the second one is defaulting to EnumConst::Const
.
I think the second one would be better at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer an assumption, we need to expose Assumptions
in a near future anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. I think the assumption is good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would also need to change the options from InlineEnumConfig
to bool
a5d46a5
to
66f3668
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
is_lhs: bool, | ||
} | ||
|
||
trait SkipInlineEnum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this trait required?
fn skip(&self) -> bool; | ||
} | ||
|
||
impl SkipInlineEnum for InlineEnum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this is the only impl.
I think it can be a method of InlineEnum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had implemented this method on another pub struct while I want to hide it, so I wrote this trait.
I am going to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some tiny issues, thank you!
|
||
pub(crate) type TSEnumLit = Rc<RefCell<AHashMap<Id, AHashMap<JsWord, Lit>>>>; | ||
|
||
pub fn inline_enum(ts_enum_lit: TSEnumLit, ts_enum_config: TSEnumConfig) -> impl Fold + VisitMut { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is exposing a crate-local type (TSEnumLit
) in public api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be pub(crate) fn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, inline_enum
should not be public API
99d6514
to
f69f138
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
LGTM.
Sorry for the delay, I had lots of PRs to review.
swc-bump:
- swc_ecma_transforms_typescript --breaking
Description:
Inline enum if possible.
BREAKING CHANGE:
Related issue (if exists):