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

Auto-fix, modernization/cleanup, and more! #6

Closed
wants to merge 16 commits into from

Conversation

apexskier
Copy link

@apexskier apexskier commented Apr 26, 2020

I started writing a different plugin, then realized there were some improvements I wanted to make to yours. Because I originally wasn't working off your source, there are a lot of changes here. I'd rather not have competing extensions in the Library, though, so I'd like to contribute to yours.

  • Add Prettier configuration, and format files with it (I've tried to match your syntax)
  • Simplify source by not using classes where necessary and using more modern import/exports
  • Upgrade all dependencies
  • Simplify build scripts
  • Switch to yarn. This is mostly person preference, but a big advantage is that it's way easier to manage merge conflicts in the lock file with it.

I've also added a few features

  • Auto-fix command
  • Auto-fix on save configuration
  • Specify path to eslint executable configuration
  • Kill running linters for a file before starting next lint pass
  • More typescript support

And I've made one change that could be controversial

I'm also planning on a few things in the future.

  • Add fallback to global eslint if installed, I just didn't get around to it, since local is much safer.
  • Don't use eslint if no config is found in workspace
  • Github actions PR tests

@jsmecham
Copy link
Owner

Hey, thanks for this! Haven’t had a chance to dive in yet, but will take a look over the next couple of days.

@gggritso
Copy link

I tried running this locally, but it refused to work! document.uri returns a string like /Users/george/my-project/file:/Volumes/Macintosh%20HD/Users/george/my-project/some-file.js and ESLint pukes up ENOENT. Changing the lintDocument function to use document.path fixed the issue, but I understand that won't work for unsaved files and so on 😢

@apexskier
Copy link
Author

@gggritso That's really strange. I haven't seen paths like that before. I just pushed a change to normalize paths before trying to lint them, can you try that?

@apexskier
Copy link
Author

apexskier commented May 18, 2020

Another option (but a largish refactor) is to use the eslint nodejs api directly (and use lintText). This would allow both linting and fixing to work on unsaved files, but there's some trickiness in getting ahold of the project-level eslint version.

@gggritso
Copy link

@apexskier I think it might have been a Nova bug! I'm on b12 now, and the code works great before and after your patch!

@StefKors
Copy link

@apexskier @gggritso I would love to use these improvements in the latest b16 version. Does it run on b16??

@StefKors
Copy link

@apexskier @gggritso @jsmecham Anything I can do to get this merged and updated in the Nova extensions library? I would love to keep 1 eslint extension instead of having multiple versions because this isn't updated.

@jsmecham
Copy link
Owner

I'll be taking a look at bringing these changes into my project shortly. I may modify things to my preferences, however.

@apexskier
Copy link
Author

Hey everyone, I just pushed some minor changes to how eslint executable setting work, to pull in some of what I've learned while writing https://github.com/apexskier/nova-typescript.

This is working fine for me on beta 17, I'm also hoping we can get it merged soon!

@atog
Copy link

atog commented Aug 26, 2020

@apexskier 's version works like a charm. Good work, thanks!

@henrikruscon
Copy link

Hows the progress on this?

@jsmecham
Copy link
Owner

I'm going to be working on this over the weekend before the release. Thank you everyone for your contributions!

@henrikruscon
Copy link

@jsmecham Looking forward to it. Let me know if you need any help.

@henrikruscon
Copy link

@apexskier Any reason the ESLint appears to be incredibly slow? Or is it a local issue for me? Testing your branch.

@apexskier
Copy link
Author

@apexskier Any reason the ESLint appears to be incredibly slow? Or is it a local issue for me? Testing your branch.

None that I know, no

@henrikruscon
Copy link

@apexskier Just to make sure, no as in you don't know what causes the slowdown or no as in you don't experience it as slow?

@apexskier
Copy link
Author

@apexskier Just to make sure, no as in you don't know what causes the slowdown or no as in you don't experience it as slow?

Both

@henrikruscon
Copy link

@jsmecham with Nova's official release being out now, you sure you don't want help to get this merged? ✌️

@jaydenseric
Copy link

@apexskier

I started writing a different plugin, then realized there were some improvements I wanted to make to yours. Because I originally wasn't working off your source, there are a lot of changes here. I'd rather not have competing extensions in the Library, though, so I'd like to contribute to yours.

Can you please publish a competing extension! Everyone I know can't use Nova because ESLint doesn't work. ESLint should always be installed and run as a project dev dependency.

@henrikruscon
Copy link

To be fair this was intended to be merged already in April this year, I think having this in a competing extension is the way to go by now.

@apexskier
Copy link
Author

I've published a fork of this extension to the Nova extension library. I've done a bit of work on top of this PR, including properly disposing of listeners (which may have been contributing to @henrikdahl's slowness issues - there was exponential growth involved on each save) and converting to typescript (which exposed a few bugs).

This was referenced Sep 18, 2020
@henrikruscon
Copy link

henrikruscon commented Sep 18, 2020

@apexskier Great work! Definitely feels faster now.

Seems that the config for fix on save is redundant right now, seems to always console the fixing message on save.

@apexskier
Copy link
Author

Filed that here - apexskier/nova-eslint#6

@apexskier apexskier closed this Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project-specific eslint doesn't work Configuration
7 participants