facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.41k stars 310 forks source link

feat: add `fix` to `eslint-plugin/sort-keys` rule #425

Closed nedjulius closed 8 months ago

nedjulius commented 9 months ago

What changed / motivation ?

Adds a fix for sort-keys ESLint rule.

This implementation has one major limitation, however. The comments are not taken into consideration for the fixes. The major reason for this is that taking care of adjacent comments introduces many edge cases and too often we cannot guarantee persistent formatting of the edited text. The current fix works like this:

Before:

const styles = stylex.create({
  foo: {
    // bar
    display: 'flex',
    // baz
    alignItems: 'center',
  },
});

After:

const styles = stylex.create({
  foo: {
    // bar
    alignItems: 'center',
    // baz
    display: 'flex',
  },
});

Let's discuss this limitation -- would like to hear some opinions of how critical you think it is to move around the comments.

Linked PR/Issues

Fixes #415

Screenshots, Tests, Anything Else

Pre-flight checklist

nedjulius commented 9 months ago

hmm, I wonder what would be the proper way to treat the comments. similar implementations that I have seen move the comments above the property along with the property. to me, it seems that the determination whether comments above and/or below the property line belong to the property context is unclear. perhaps it's something that consumer could control via rule options.

another issue is formatting. it seems that it's almost impossible to write a fixer that maintains the correct formatting after moving comments in every edge case. I wonder if ESLint --fix takes care of indentation and whitespace with or without formatting tool dependency -- will try this out.

nmn commented 9 months ago

Maintaining formatting is less important that the comments IMO.

nedjulius commented 9 months ago

alright, I improved the fixer to handle the comments and added multiple test cases to test out how it works. there is a big chance that I might have missed some possible test cases; I would appreciate any input.

here's a few assumptions that I've made:

  1. comments that start on the same line as the impacted property are always treated as part of the property context
    const styles = stylex.create({
    foo: {
    display: 'flex', // this comment will be moved along with the "display" property
    alignItems: 'center' // this comment will be moved along with the "alignItems" property
    },
    });
  2. comments that are above the impacted property, and do not share the line with any other token, are always treated as part of the property context
    const styles = stylex.create({
    foo: { /* this block comment is not part of the "display" context
    foo
    bar
    */
    // this comment will be moved along with the "display" property
    display: 'flex',
    // this comment will be moved along with the "alignItems" property 
    alignItems: 'center'
    },
    });
nmn commented 9 months ago

Just one thing to double check:

const styles = stylex.create({
  foo: { /* this block comment is not part of the "display" context
    foo
    bar
    */
    // this comment will be moved along with the "display" property
    // And this comment?
    display: 'flex',
    /*
      this comment will be moved along with the "alignItems" property 
    */
    alignItems: 'center'
  },
});

If this is handled correctly, the assumptions make sense.

nedjulius commented 9 months ago

@nmn yup, both comments will be included in the move; the autofix will output:

const styles = stylex.create({
  foo: { /* this block comment is not part of the "display" context
    foo
    bar
    */
    /*
      this comment will be moved along with the "alignItems" property 
    */
    alignItems: 'center',
    // this comment will be moved along with the "display" property
    // And this comment?
    display: 'flex',
  },
});

basically, all comments above the property that do not share the line with any other token or node will be included

nmn commented 9 months ago

Great! Sounds like it'll work. I'll do some extra testing and merge it for the next release.