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

add eslint@9 support and flat config #3942

Merged
merged 9 commits into from
Oct 20, 2024

Conversation

dartess
Copy link
Contributor

@dartess dartess commented Oct 16, 2024

Code change checklist

  • Added/updated unit tests
  • Updated /docs. For new functionality, at least API.md should be updated
  • Verified that there is no significant performance drop (yarn mobx test:performance)

Closed #3856

  • added compatibility with eslint@9;
  • added support for flat config;
  • added recommended config in flat format;
  • added documentation for using with flat config;
  • added tests to check rules in legacy (using eslint@7) and in modern (eslint@9) environment.
  • everything is tested on my own project with flat config;

Copy link

changeset-bot bot commented Oct 16, 2024

🦋 Changeset detected

Latest commit: 0d03c33

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-mobx Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dartess dartess changed the title add eslint@9 and flat config add eslint@9 support and flat config Oct 16, 2024
@dartess dartess changed the title add eslint@9 support and flat config WIP: add eslint@9 support and flat config Oct 16, 2024
@dartess dartess marked this pull request as draft October 16, 2024 07:11
@iChenLei iChenLei marked this pull request as ready for review October 16, 2024 07:14
@iChenLei
Copy link
Member

I'm sorry, I accidentally clicked the button and removed the Draft mode

@dartess
Copy link
Contributor Author

dartess commented Oct 16, 2024

@iChenLei moved to draft to add changeset; already done

upd: no, still a draft, I'll try to fix the tests in CI.

@dartess dartess changed the title WIP: add eslint@9 support and flat config add eslint@9 support and flat config Oct 16, 2024
@dartess dartess marked this pull request as draft October 16, 2024 07:17
@dartess
Copy link
Contributor Author

dartess commented Oct 16, 2024

I think that's all 🙌

@dartess dartess marked this pull request as ready for review October 16, 2024 07:56
@urugator
Copy link
Collaborator

urugator commented Oct 19, 2024

There is a preview directory, that provides "live" (you need to reload window in between changes) IDE preview . This is super useful when developing a rule (if you know of better way, let me know). Since you removed "eslint": "^7.0.0" from dev deps, I assume the IDE will use eslint installed at the root of the mobx repo, which is "eslint": "^6.8.0", . Can we make sure it uses 7 or 9? I think we don't need preview for both at the moment.

@dartess dartess marked this pull request as draft October 19, 2024 17:11
@dartess
Copy link
Contributor Author

dartess commented Oct 19, 2024

There is a preview directory, that provides "live" (you need to reload window in between changes) IDE preview . This is super useful when developing a rule

Wow! I didn't realize this was used during rules developing. This looks nice!

if you know of better way, let me know

In fact, I see a flaw in this way. It is essentially a manual way of testing. It can be really convenient during development, but at the end of development, all preview examples (in a good way) should be copied to test cases. However, this can be forgotten, and all this is code duplication (what is bad).

Usually I've seen a simpler way - development through test cases in observation mode. I can add something like "test:watch" to the scripts. It's not as convenient, but it just works and doesn't require a "preview" dir at all.

I assume the IDE will use eslint installed at the root of the mobx repo, which is "eslint": "^6.8.0"

Yes, that's true, at least for JetBrains IDE. I didn't take that into account either.

Option 1: remove "preview" folder for development and switch to tests and watch mode (to avoid the need to synchronize preview and __tests__) (I prefer this)

Option 2: configure preview with eslint@7 (just return it to how it was)

Option 3: configure preview with eslint@9 (all versions below 9 are officially deprecated now, so this makes sense, but there may be problems with switching between different configs for editors and IDE)

Your opinion?

@urugator
Copy link
Collaborator

urugator commented Oct 19, 2024

It is essentially a manual way of testing

Let me aswer with some rhetorical questions:
Have you ever developed an application just by writing tests?
At the time of writing, eslint supported multiple suggestions/fixes, but vscode couldn't handle that, how would you know?
In the OP, you're mentioning "everything is tested on my own project with flat config;". So automated tests did not suffice?

Your opinion?

I am fine with both 2/3 as long as it works in vscode :D Your call.

@dartess
Copy link
Contributor Author

dartess commented Oct 20, 2024

yes, these are good questions.

I rollback eslint@7 as dev deps, now it should work as before.

@dartess dartess marked this pull request as ready for review October 20, 2024 07:06
@urugator urugator merged commit 218ebde into mobxjs:main Oct 20, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Oct 20, 2024
@urugator
Copy link
Collaborator

Thank you for looking into it. Much appreciated.

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.

[eslint-plugin-mobx] ESLint v9 Support
3 participants