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: tailwind variant sorting #3208

Merged
merged 11 commits into from
Jun 24, 2024
Merged

Conversation

lutaok
Copy link
Contributor

@lutaok lutaok commented Jun 13, 2024

Summary

Hello, I'm opening this PR to close Variant Sorting from #1274 .
Huge thanks to @DaniGuardiola for kicking this off.
I'm opening it as a draft because there's some things that I'm not handling yet, like:

  • Arbitrary/Dynamic Variants (has-[], group-[]) -> I spotted where I can take this info from the tailwind config, but I think I have to tweak tokenize_class function in order to take them into consideration and this is what I plan to do next.
  • Screen Sorting -> I need to take into consideration the screens sorting function in order to have the same output as the current plugin (Won't do in this PR)
  • More tests -> Left some TODOs for get_class_info_tests, since, by introducing bitvec, I have to exactly know which BitVec is produced for a certain class. Or is it finding that from tailwind preset acceptable in a test environment?

I would also like to include these two things, but they could be inserted on some new PRs:

  • Negative Values (Won't do in this PR)
  • Custom separator (Won't do in this PR)

Test Plan

My tests are manual by letting run the rule on jsx elements and by comparing the diagnostic result with a TailwindCSS Sorter Plugin playground output.

I am slowly getting to a clearer understanding of the issue, but if you feel you might solve this faster, I will gladly pass the torch 😁 I know it's a much requested feature and I will also benefit from this, I wouldn't want to slow things down with my inexperience with Rust or Tailwind Sorting inner workings!

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Jun 13, 2024
@lutaok lutaok changed the title feat: tailwind variant sorting feat: tailwind variant sorting (WIP) Jun 13, 2024
@DaniGuardiola
Copy link
Contributor

Thank you!!! I'll be happy to review and help with anything. This week is gonna be busy (traveling for work), but I'll try next week. Regarding the TODOs, well spotted but I think we can progressively enhance the sorting algorithm, so I wouldn't consider them blocking. Just having some basic variant sorting based on weights is already great progress 👍

Copy link

codspeed-hq bot commented Jun 13, 2024

CodSpeed Performance Report

Merging #3208 will not alter performance

Comparing lutaok:feature/tailwind-sorting (bb82060) with main (86d4e7c)

Summary

✅ 90 untouched benchmarks

Copy link
Contributor

@DaniGuardiola DaniGuardiola left a comment

Choose a reason for hiding this comment

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

Looking pretty good! Left a few questions and minor nits throughout. I haven't done a deep full pass yet fyi.

Q: have you made sure that the output conforms to rustfmt, or does it need to be run separately?

Regarding the TODOs in the description, I think we can ignore all of them (except the unit tests) for now for the scope of this PR, as they are not trivial and this is already a ton of progress.

@@ -572,11 +573,684 @@ const UTILITIES_LAYER_CLASSES: [&str; 569] = [
"duration-",
"ease-",
"will-change-",
"contain-none$",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these coming from? AFAIK, this is a plugin and the codegen script works with a clean, empty config. I couldn't find the source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am just doing just gen-tw in order to execute the script and generate the preset, I had to install bun and bun install the package, so maybe those were included in some newer version of TailwindCSS?

"content-",
"forced-color-adjust-auto$",
"forced-color-adjust-none$",
];

pub fn get_variant_classes() -> Vec<Variant> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pinging @ematipico for perf advice here. What is needed is a map between strings that identify variants and their respective "weights". A weight is a bitvec, and the way it works is the following.

Variants exist in a specific order. E.g. var1, var2 and var3.

For each entry, we need to compute a weight which is a bit vector of 1 shifted n times, n being the index in the ordered list I mentioned. So in our example, that'd be:

  • var1: 1
  • var2: 10
  • var3: 100

And so on. Tailwind classes can have multiple variants, e.g. var1:var3:m-4. When that happens, those bit vectors are xor'd, like this: 1 xor 100 = 101. With the resulting bit vector, classes are sorted.

I would love your input in how to optimize this. We can either just find a way to reduce computation, or memory consuption, etc - or we could even figure out some smart shortcut that has the same result as this xor thing but better perfomance. Any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pinging @ematipico for perf advice here. What is needed is a map between strings that identify variants and their respective "weights". A weight is a bitvec, and the way it works is the following.

Variants exist in a specific order. E.g. var1, var2 and var3.

For each entry, we need to compute a weight which is a bit vector of 1 shifted n times, n being the index in the ordered list I mentioned. So in our example, that'd be:

  • var1: 1
  • var2: 10
  • var3: 100

And so on. Tailwind classes can have multiple variants, e.g. var1:var3:m-4. When that happens, those bit vectors are xor'd, like this: 1 xor 100 = 101. With the resulting bit vector, classes are sorted.

I would love your input in how to optimize this. We can either just find a way to reduce computation, or memory consuption, etc - or we could even figure out some smart shortcut that has the same result as this xor thing but better perfomance. Any ideas?

I don't know if this changes the math a bit, but just want to point out here too that I've seen that n as the times 1 is shifted isn't strictly the index of the generated array of variants.
It will probably work well for single variants classes, but when it comes to doing bitwise XOR, I think the result would change.
Haven't tested it out yet and I'm of course interested in any way that could make this more performant 😄

Copy link
Member

Choose a reason for hiding this comment

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

Are there limitations on the number of weights that a variant can store? For example, is it realistic to have 1000 variants? Having 1000 variants would mean that our highest base weight would be 1000*100, plus the singular variations.

Also, do we expect get_variant_classes to receive variables? If not, we should make sure to create these variants only once. This is achievable with OnceCell or with lazy_static!, depending on the use case. Use OnceCell if the function needs to accept arguments, use lazy_static! otherwise.

Another piece of optimisation. Use smallvec instead of a Vector. The vector tends to save info inside the heap, but if you know the number of the variants like in this case, smallvec will save the data in the stack.

Copy link
Contributor Author

@lutaok lutaok Jun 17, 2024

Choose a reason for hiding this comment

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

Thanks for the reply!

Are there limitations on the number of weights that a variant can store? For example, is it realistic to have 1000 variants? Having 1000 variants would mean that our highest base weight would be 1000*100, plus the singular variations.

Also, do we expect get_variant_classes to receive variables? If not, we should make sure to create these variants only once. This is achievable with OnceCell or with lazy_static!, depending on the use case. Use OnceCell if the function needs to accept arguments, use lazy_static! otherwise.

Each variant has its own weight, which right now has a max at a BitVec with 166 elements inside for the print variant.
Tailwind accepts user-defined variants and those will be put after the default ones unless you override the whole default order configuration: Custom Variant Ordering

I haven't dug deeper on user-defined variants and their ordering and thus is not included in this PR, but it's safe to assume there will be no limit to the number of variants.

I don't know if this changes the math a bit, but just want to point out here too that I've seen that n as the times 1 is shifted isn't strictly the index of the generated array of variants.
It will probably work well for single variants classes, but when it comes to doing bitwise XOR, I think the result would change.

I also have to retract this in some way, I've noticed two jumps on weight:

  • between marker and selection
  • between selection and file
    and that should be because on variantsMap (which I'm not using) they have duplicates on marker and selection variants.

I think it's safe to assume they're strictly progressive and the XOR operation wouldn't be impacted by generating BitVectors on the fly.

Another piece of optimisation. Use smallvec instead of a Vector. The vector tends to save info inside the heap, but if you know the number of the variants like in this case, smallvec will save the data in the stack.

Will try to use smallvec and lazy_static as well, I don't expect any arguments to be passed to the highlighted function.

Copy link
Contributor

Choose a reason for hiding this comment

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

unless you override the whole default order configuration

We can disregard this. AFAIK, this API doesn't exist anymore. The only way to alter the order in any way is through the before option in plugins, but the tailwind team confirmed to me that this is a legacy API and it's likely going away soon. Copying my question and their answer here:

Q: Are utilities, components and variants added by (non-built-in) plugins ALWAYS indexed after the built-in ones?

A: Almost. The general answer is yes. There's a before option for variants which allows you to register it before another one but we consider it a legacy thing.


function introspectVariants(context: TailwindContext): Set<VariantSpec> {
const variants = new Set<VariantSpec>();
// This method returns a list of Variants, each with values but offsets are missing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This method returns a list of Variants, each with values but offsets are missing
// This method returns a list of Variants, each with values but offsets are missing.

const variants = new Set<VariantSpec>();
// This method returns a list of Variants, each with values but offsets are missing
const configVariants = context.getVariants();
// This Map contains weights for each variant name, including those that are values of a main variant
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This Map contains weights for each variant name, including those that are values of a main variant
// This Map contains weights for each variant name, including those that are values of a main variant.


function buildConfigVariants(spec: TailwindSpec): Array<Variant> {
const variants: Array<Variant> = [...spec.variants]
.sort((variantA, variantB) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do implicit return, and name a and b for consistent naming like in the utility code above

// TODO: return None if there is an unknown variant.
Some(0) // TODO: actually compute variant weight
let known_variants = utility_data.variants.iter().any(|variant| {
sort_config
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably just be a hashmap access of sorts for speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on this, I went with this to not revolutionize everything
I feel that a hashmap structure would then have a value about arbitrariety and could not be a simple HashMap<string, BitVec>, am I right?

Copy link
Contributor

@DaniGuardiola DaniGuardiola Jun 14, 2024

Choose a reason for hiding this comment

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

My hope regarding variants with arbitrary values is that we won't need to make a distinction between variants that support it and variants that don't, and we'll be able to just take a "starts with" kind of approach for simplicity, since the only significant trade-off there is the possibility of false positives which is very minor. This is similar to how utilities are handled and it seems to work well enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably endsWith?
I would then have to generate for example has-[] instead of has that is currently generated (same as group-[] instead of group)
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went reading Tailwind docs more and testing the plugin more about this.
My previous comment doesn't apply at all 😄
But wanting to point out that group-[:checked] keeps group weight when sorting, but group-hover has its own weight. From the generated Variants there's also group-aria-busy for example that has its own weight. By doing starts_with, since it comes after group in the Vec<Variants> we would have a false positive when the iterating step is on group

I think we should do the matching logic this way:

  • Does the variant on the classes endsWith "]" AND its prefix (as a whole though) is a recognized variant? if yes, then it's a known variant and we can compute weights
  • Keeping the same logic on the exact match between variants names

I've also noticed that [&nth-child(2)]:py-4 are put right before everything, before custom classes or components layer utility classes, while the plugin puts them right after every recognized variant and orders them lexicographically, so that's another thing to tackle.

Copy link
Contributor

@DaniGuardiola DaniGuardiola Jun 19, 2024

Choose a reason for hiding this comment

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

Check out how I handle this situation in utilities. You'll see that there's logic that gives priority to longer matches over shorter ones. I think that should probably work here too and be quite efficient.

// Multiple partial matches can occur, so we need to keep looking to find
// the longest target that matches. For example, if the utility text is
// `gap-x-4`, and there are targets like `gap-` and `gap-x-`, we want to
// make sure that the `gap-x-` target is matched as it is more specific,
// regardless of the order in which the targets are defined.
let target_size = target.chars().count();
if target_size > last_size {
layer = layer_data.name;
match_index = index;
last_size = target_size;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yes I copied your function because the structure of the variants is somewhat different, but I wonder if we could make it reusable enough

@@ -17,3 +20,10 @@ pub fn get_utilities_preset(preset: &UseSortedClassesPreset) -> UtilitiesConfig
UseSortedClassesPreset::TailwindCSS => TAILWIND_LAYERS.as_slice(),
}
}

pub fn get_variants_preset(preset: &UseSortedClassesPreset) -> VariantsConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we can unify into a "preset" struct or something for simplicity, instead of two separate things as currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah probably we can lift the split on kind of preset, instead of this, I will give it a go

@@ -32,10 +32,21 @@ impl ClassInfo {
None
}

/// Compare based on variants weight. Classes with higher weight go first.
/// Compare based on variants weight. Classes with lower weight go first.
/// First compare variants weight length. Only if their equal compare their actual weight.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really faster? We should benchmark because I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These steps are taken because bitvec's cmp doesn't actually compare lengths to determine which one is greater or less.

I then modified the lower weight go first because I've seen from the output of the current Prettier plugin that this the order they have on for example hover and focus. hover will always come first because its weight is 1000 and focus's is 10000. (the values are not realistic, but it's just to show how they differ)

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is that I would be surprised if just comparing in the way that's native to bitvec isn't faster than this manual approach. Are we sure that's the case? Maybe it is, I just think it's important that we ensure that because otherwise this is a premature optimization that is actually counterproductive. Know what I mean? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I probably didn't explain it well enough. I resorted to this because I tried by letting the native bitvec cmp method to handle comparison but it didn't work as I expected. It seems it doesn't check the BitVecs length first to establish which one is greater than the other.
I don't actually know what it does under the hood, I haven't checked yet.
I'm not well versed enough with Rust to think about optimizations first, my solution wasn't about speed 😁

pub weight: BitVec<u8, Lsb0>,
}

/// This builds a bit vector ordered from Lsb to Msb in order to perform a simpler BitWise XOR later on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// This builds a bit vector ordered from Lsb to Msb in order to perform a simpler BitWise XOR later on
/// Builds a bit vector ordered from Lsb to Msb in order to perform a simpler BitWise XOR later on.

}

/// This builds a bit vector ordered from Lsb to Msb in order to perform a simpler BitWise XOR later on
/// Every variant has one bit set to 1 (Msb) and the others set to 0 and they have fixed size
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this comment is that relevant, at least not as docs for the function - it's an implementation detail. FWIW, I intend to write about the Tailwind sorting algorithm in general so that could be used as reference for anyone trying to achieve a technical understanding of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just wanted to document in the code about the why this approach has been taken. I'm down to remove it or to actually link to any external reference in order to understand it further

@DaniGuardiola
Copy link
Contributor

Thinking out loud. What if we store variants in a similar way to utilities, and only when comparing we compute the weights on the fly (and cache them)?

With how utilities are currently declared in the preset, it's very efficient - we can do a similar matching algorithm to get indexes for variants. From the indexes, we can compute those weights. There might even be a shortcut from the indexes that allows us to skip bit vectors entirely, though I haven't thought about the math yet.

In any case it seems like a more efficient approach to me, unless we can make bit vectors static and precompute them somehow. I'm too much of a Rust noob to know :P

@lutaok
Copy link
Contributor Author

lutaok commented Jun 14, 2024

Looking pretty good! Left a few questions and minor nits throughout. I haven't done a deep full pass yet fyi.

Q: have you made sure that the output conforms to rustfmt, or does it need to be run separately?

Regarding the TODOs in the description, I think we can ignore all of them (except the unit tests) for now for the scope of this PR, as they are not trivial and this is already a ton of progress.

My check on rustfmt was by running cargo format on the generated file and no changes were made, so I guess it is conform already (?)

@lutaok
Copy link
Contributor Author

lutaok commented Jun 14, 2024

Thinking out loud. What if we store variants in a similar way to utilities, and only when comparing we compute the weights on the fly (and cache them)?

With how utilities are currently declared in the preset, it's very efficient - we can do a similar matching algorithm to get indexes for variants. From the indexes, we can compute those weights. There might even be a shortcut from the indexes that allows us to skip bit vectors entirely, though I haven't thought about the math yet.

In any case it seems like a more efficient approach to me, unless we can make bit vectors static and precompute them somehow. I'm too much of a Rust noob to know :P

My first approach was trying to make bitvecs static, but I got yelled at by the Rust Compiler, that's why I resorted on using a Function that would allocate those BitVecs.

Your proposed solution seems efficient, but from what I've seen, each variant weight isn't strictly progressive on its length, and I didn't seem to find any pattern to it. I wouldn't know how big should the BitVec be in that case

@lutaok
Copy link
Contributor Author

lutaok commented Jun 14, 2024

Just wanted to add thank you @DaniGuardiola for your fast review and your suggestions, I instantly got to the replies and forgot manners 😁

@lutaok lutaok marked this pull request as ready for review June 22, 2024 19:43
@lutaok
Copy link
Contributor Author

lutaok commented Jun 22, 2024

Marking this PR as public because I pushed some new work that:

  • Supports Arbitrary variants (e.g [&nth-child(2)]) sorting reliably.
  • Supports Variants that can have custom values like has-[:checked] or group-[.custom-class] and implemented a similar match from Utilities in order to avoid false positives (hope so at least)

I also added more unit tests and of course manual tested that my outputs were the same as the Prettier Plugin ones.

For Variants, there are two remaining things that I haven't looked into:

  • Screen Sorting
  • peer and group sorting which I think are called parasite utilities (maybe I misunderstood here), but I don't feel they will work out without managing them explicitly.

@lutaok lutaok changed the title feat: tailwind variant sorting (WIP) feat: tailwind variant sorting Jun 22, 2024
@DaniGuardiola
Copy link
Contributor

@lutaok parasite utilities are simply utilities that "don't exist", i.e. they don't output any tailwind code, and only exist to aid selectors in other variants/utilities that do (e.g. group-hover:). Don't worry about them, we'll probably just need to add them statically to the tailwind preset gen script.

Regarding screen sorting, my idea was to just have a copy of the config in the biome json, and basically reimplement the existing sort function. There is a bit of complexity, as there are multiple units and ways to specify screens. Let's do it in a follow-up though.

@lutaok
Copy link
Contributor Author

lutaok commented Jun 23, 2024

@lutaok parasite utilities are simply utilities that "don't exist", i.e. they don't output any tailwind code, and only exist to aid selectors in other variants/utilities that do (e.g. group-hover:). Don't worry about them, we'll probably just need to add them statically to the tailwind preset gen script.

Oh okay, got it, thanks!

Regarding screen sorting, my idea was to just have a copy of the config in the biome json, and basically reimplement the existing sort function. There is a bit of complexity, as there are multiple units and ways to specify screens. Let's do it in a follow-up though.

Yeah I agree on implementing it in a follow up PR.
Thanks for the info!

Comment on lines 296 to 298
impl VariantMatch {
/// Checks if a variant matches a target, and returns the result.
fn from(target: &str, variant_text: &str) -> VariantMatch {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could implement using the From trait like this:

impl From<(&str, &str)> for VariantMatch {
	fn from((target, variant): (&str, &str)) -> Self {

	}
}

This is way better, and you can things like ("s-md", "s-md").into()

};

// if it has a custom value thus it starts with the variant text and it's followed by "-["
if variant_text.starts_with(format!("{}-[", target).as_str()) {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

possible optimisation for the future: we should not allocate a new string, use chars() check {}-[ using an iterator.

}
}

fn find_variant_info(config_variants: VariantsConfig, variant_text: &str) -> Option<usize> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: since this function returns a number, maybe we could give _info a better name. In other words, what's "info"? What does the number we return represent?

}

fn find_variant_info(config_variants: VariantsConfig, variant_text: &str) -> Option<usize> {
let mut variant: &str = "<no match>";
Copy link
Member

@ematipico ematipico Jun 23, 2024

Choose a reason for hiding this comment

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

Move "<no match>" to a const outside of this function. There's no need to create a placeholder each time we can this function.

Another alternative, use a let mut variant: Option<&str> = None instead, so we don't need to use a string no match at all.

last_size = target_size;
}
}
_ => {}
Copy link
Member

Choose a reason for hiding this comment

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

Small advice I found useful: don't use _ inside a match, because if the future you add a new variant, you won't get compiler error.

Comment on lines 56 to 65
if self.arbitrary_variants.is_some() && other.arbitrary_variants.is_some() {
return None;
}
if self.arbitrary_variants.is_some() {
return Some(Ordering::Greater);
}
if other.arbitrary_variants.is_some() {
return Some(Ordering::Less);
}
None
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this should be a match, e.g.

match (a, b) {
	(Some(_), Some(_)) => None,
	(_, Some(_)) => Some(Ordering::Greater)
}

Comment on lines 70 to 71
let a = self.arbitrary_variants.clone()?;
let b = other.arbitrary_variants.clone()?;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I don't think we need to clone

@@ -56,6 +98,16 @@ impl ClassInfo {
// This comparison function follows a very similar logic to the one in Tailwind CSS, with some
// simplifications and necessary differences.
fn compare_classes(a: &ClassInfo, b: &ClassInfo) -> Ordering {
// Classes with arbitrary variants go last
if let Some(has_arbitrary_variants) = a.cmp_has_arbitrary_variants(b) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: a method called *has should return a bool, logically speaking. Maybe we should give a better name

"content-",
"forced-color-adjust-auto$",
"forced-color-adjust-none$",
];

lazy_static! {
pub static ref VARIANT_CLASSES: [&'static str; 165] = [
Copy link
Member

Choose a reason for hiding this comment

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

Arrays don't need to be initialized with lazy static. If you ended using lazy static, it means that there's a refactor opportunity. This could be pub const like other arrays

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arrays don't need to be initialized with lazy static. If you ended using lazy static, it means that there's a refactor opportunity. This could be pub const like other arrays

Facepalmed myself after realizing that. Thank you for pointing that out!

@lutaok
Copy link
Contributor Author

lutaok commented Jun 23, 2024

Hi @ematipico , thanks for the super insightful review!
I think I've addressed all the review comments 😁

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

You got rid of the clone, that's amazing! Amazing job @lutaok, and thank you very much for taking over this complex task!

@ematipico
Copy link
Member

@DaniGuardiola, I am going to merge this but feel free to leave any reviews/comments, and we will address them later.

Same for you, @lutaok, I am going to merge this, but it would amazing if you could follow up with PR to update the changelog with a summary of the change you've done and maybe update the documentation of the rule, if it's needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants