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

Feature: Support Logical Ordering for Specific String Literal Types #408

Closed
2 tasks done
akibarika opened this issue Dec 2, 2024 · 16 comments · Fixed by #411
Closed
2 tasks done

Feature: Support Logical Ordering for Specific String Literal Types #408

akibarika opened this issue Dec 2, 2024 · 16 comments · Fixed by #411
Labels
enhancement New feature or request

Comments

@akibarika
Copy link

akibarika commented Dec 2, 2024

Describe the rule

In some cases, string literal types in TypeScript are better represented in a specific logical order rather than an alphabetical one. For example:

type Breakpoint = 'xs' | 'sm' | 'md' | 'lg' | 'xl'; // Logical order

In this case, the progression from 'xs' (extra small) to 'xl' (extra large) follows a clear semantic or domain-specific logic. However, the current sorting rules in eslint-plugin-perfectionist would reorder these alphabetically:

type Breakpoint = 'lg' | 'md' | 'sm' | 'xl' | 'xs'; // Alphabetical order

This disrupts the intended meaning and reduces the readability of such types, which are often used in design systems, configuration objects, or domain-specific types.

Code example

  1. Design Systems: Breakpoints in CSS frameworks ('xs', 'sm', 'md', 'lg', 'xl').

  2. User States: User states in a workflow ('pending', 'active', 'completed', 'archived').

  3. Error Severity Levels: Severity levels ('low', 'medium', 'high', 'critical').

  4. Task Priorities: Task priority levels ('low', 'normal', 'high', 'urgent').

  5. Font size: thin, light, regular, bold.

Additional comments

No response

Validations

  • Read the docs.
  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
@akibarika akibarika added the enhancement New feature or request label Dec 2, 2024
@hugop95
Copy link
Contributor

hugop95 commented Dec 2, 2024

Hi @akibarika

This is similar to #148 and #98.

I like this feature a lot, but I'm having trouble understanding the real use case:
Let's use your example

type Breakpoint = 'xs' | 'sm' | 'md' | 'lg' | 'xl'; // Logical order

[Question] Are there multiple cases where a user would need to define this specific logical order? If not, why not just disable the rule with eslint-disable for this type?

Edit: This feature makes actually sense in objects.

I see 2 challenges to tackle:

  • Defining multiple configurations (one per "logic").
  • Knowing when to use which configuration.

Point 1: Defining multiple configurations

At first sight, not technically complicated through options.

Point 2: Knowing when to use which configuration

This is where this gets a big harder. I don't think there is a way to automatically detect the appropriate logic to use based on code context alone.

One potential solution is to implement #400 with custom annotation comments.

Assuming that a logic configuration named sizeLogic is defined:

// @perfectionist/sort-objects { configuration: 'sizeLogic' }
const sizeNames = {
  xs: 'XS',
  s: 'S',
  m: 'Medium',
  l: 'Larger',
  xl: 'XL'
}

@akibarika
Copy link
Author

Hi @hugop95 ,

Thank you for the quick response!

I agree with your points, and I also think it’s worth implementing this feature for objects.

Here’s a workaround I’ve been using:

"rules": {
  "perfectionist/sort-objects": [
    "warn",
    {
      "groups": ["xs", "sm", "md", "lg", "xl", "unknown"],
      "customGroups": {
        "xs": "xs",
        "sm": "sm",
        "md": "md",
        "lg": "lg",
        "xl": "xl"
      }
    }
  ]
}

It’s not a perfect solution and has some limitations, but it does work for now.

@hugop95
Copy link
Contributor

hugop95 commented Dec 3, 2024

I don't think there is a way to automatically detect the appropriate logic to use based on code context alone.

Thinking about it, there is actually a way if we consider that most "logical" groups are a set of specific keys: we can use the fact that users can pass multiple configurations to a single rule.

For sort-objects in particular (the rule that benefits the most from this), this means that we could pass

[
  {
    groups: ['r', 'g', 'b'],
    customGroups: [
      {
        groupName: 'r',
        elementNamePattern: 'r',
      },
      {
        groupName: 'g',
        elementNamePattern: 'g',
      },
      {
        groupName: 'b',
        elementNamePattern: 'b',
      },
    ],
    useConfigurationIf: {
      allElementNamesMatchPattern: '^r|g|b$',
    },
  },
  {}, // Fallback configuration with default rule options
],

This means that

let obj = {
  r: string,
  g: string,
  b: string,
}

would be correctly sorted by rgb logic, while other types would use the fallback configuration.

@OlivierZal What do you think?

@OlivierZal
Copy link
Contributor

OlivierZal commented Dec 3, 2024

Hi @hugop95,

I agree with you that the best place to configure it should be the customGroups.

Here are some considerations though.

First, I think that the semantical sorting topic is legitimate: another common use case which comes to my mind is min | max.

However, I also see possible consistency issues with sorting.

e.g. when there are only min and max, it's ok:

interface Range {
  min: number
  max: number
}

but what is the expected behavior when it's mixed with other keys, e.g.

interface HTMLCustomElement {
  id: string // let's say it's configured to be first
  a: string
  max: string
  mid: string
  min: string
  z: string
  other: string // let's say it's configured to be last
}

I agree that customGroup is flexible enough to handle such a case, but maybe with some adaptation – given that:

  • some "semantical" keys could be mixed with other keys;
  • some key could sometimes miss, but I still expect the key to be placed where it's configured to be, e.g. a range key right after id (min, even if max is missing) – that's also the point of semantical sorting: semantics is not only "internal" (within a given group) but also "external" (should be known to other groups as well);
  • some groups could also theoretically have an infinite number of elements (even if they actually have a finite one);

That's why I propose a solution which is not limited to useConfigurationIf: allElementNamesMatchPattern value:

[
  {
    groups: ['rgb'],
    customGroups: [
      {
        groupName: 'rgb',
        elementNamePattern: '^r|g|b$',
        internalGroupOrder: ['r', 'g', 'b'],
      },
    ],
  },
],

so that this:

[
  {
    groups: ['id', 'range', 'unknown', 'other'],
    customGroups: [
      {
        groupName: 'id',
        elementNamePattern: '^id$',
      },
      {
        groupName: 'range',
        elementNamePattern: '^(min|max)',
        internalGroupOrder: ['^min', '^max'],
      },
      {
        groupName: 'other',
        elementNamePattern: '^other$',
      },
    ],
  },
],

would result in this:

interface HTMLCustomElement {
  id: string
  minIndoorTemperature: string
  minOutdoorTemperature: string
  maxIndoorTemperature: string
  a: string
  z: string
  other: string
}

and this:

[
  {
    groups: ['id', 'range', 'unknown', 'other'],
    customGroups: [
      {
        groupName: 'id',
        elementNamePattern: '^id$',
      },
      {
        groupName: 'range',
        elementNamePattern: '^(min|max)',
        internalGroupOrder: ['IndoorTemperature$', 'OutdoorTemperature$'],
      },
      {
        groupName: 'other',
        elementNamePattern: '^other$',
      },
    ],
  },
],

in this:

interface HTMLCustomElement {
  id: string
  minIndoorTemperature: string
  maxIndoorTemperature: string
  minOutdoorTemperature: string
  a: string
  z: string
  other: string
}

What do you think @hugop95?

@hugop95
Copy link
Contributor

hugop95 commented Dec 3, 2024

@OlivierZal

I like your proposal a lot, I think that the complexity justifies implementing it as another issue (considering that my proposal and yours are independent).

You are essentially adding another layer of grouping inside a group. Rather than adding a internalGroupOrder attribute that is limited to name pattern matching, we can simply re-use groups:

Your last example could become

interface HTMLCustomElement {
  id: string
  minIndoorTemperature: string
  maxIndoorTemperature: string
  minOutdoorTemperature: string
  a: string
  z: string
  other: string
}
[
  {
    groups: ['id', 'range', 'unknown', 'other'],
    customGroups: [
      {
        groupName: 'indoor', // Only used within the `range` group
        elementNamePattern: 'IndoorTemperature$'',
      },
      {
        groupName: 'outdoor', // Only used within the `range` group
        elementNamePattern: 'OutdoorTemperature$'',
      },
      {
        groupName: 'id',
        elementNamePattern: '^id$',
      },
      {
        groupName: 'range',
        elementNamePattern: '^(min|max)',
        groups: ['indoor', 'outdoor'],
      },
      {
        groupName: 'other',
        elementNamePattern: '^other$',
      },
    ],
  },
],

This is a bit related to #68, where users want to colocate things together.

@OlivierZal
Copy link
Contributor

@hugop95, you got my point, I think we are on the same page 😊

@hugop95
Copy link
Contributor

hugop95 commented Dec 3, 2024

At first glance unfortunately, I don't think that's not something we can easily add, this will deserve its own issue 🙂

@akibarika
Copy link
Author

Thank you @hugop95 & @OlivierZal !!

I've just updated to the latest version, 4.2.0, but it seems to not be working.

Value {"groups":["r","g","b"],"customGroups":{"r":"r","g":"g","b":"b"},"useConfigurationIf":{"allNamesMatchPattern":"^r|g|b$"}} should NOT have additional properties.

It looks like the change hasn’t been included in the latest release yet.

@hugop95
Copy link
Contributor

hugop95 commented Dec 9, 2024

The feature hasn't been released yet indeed. It will be here in 4.3.0 soon.

@OlivierZal
Copy link
Contributor

And thank only @hugop95, he did the whole work!

@h0adp0re
Copy link

Thanks for this feature, it looks like exactly what I need! I’m just curious about your release process. Do you have a timeline for when this might be published?

At my workplace, we typically publish features right after merging a PR. The thinking is — if time and energy have been spent on building a feature, it makes sense to make it available as soon as possible for everyone to use. It seems to help keep things moving smoothly, but I understand there are different approaches depending on the project.

@hugop95
Copy link
Contributor

hugop95 commented Dec 12, 2024

@h0adp0re The release process is handled by @azat-io, who can better explain the approach for this project.

In general:

  • Critical hotfixes are released ASAP.
  • Low-impact bugs and features tend to be grouped together in a release. There is another big feature about to be merged, so both features might end up in the same release.
  • I think you can expect the release to be shipped by the end of the week.

@h0adp0re
Copy link

@hugop95 Wow, great job on the alphabet. And thanks for the outline.

I've noticed this feature grouping behavior in other open-source projects before and I have to say I don't quite get it 😄 — what if the feature being waited after takes several weeks more to refine? I am not here to argue or anything, just curious about the rationale behind the indeterminate waiting.

@azat-io
Copy link
Owner

azat-io commented Dec 12, 2024

@h0adp0re Hello! I will publish the release today

@azat-io
Copy link
Owner

azat-io commented Dec 12, 2024

Added in a3ee3ff.
Released in v4.3.0.

@h0adp0re
Copy link

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants