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

Migrate to Typescript #29

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

daymxn
Copy link
Collaborator

@daymxn daymxn commented Dec 5, 2024

This PR migrates the plugin/rule to typescript, alongside a variety of other "minor" changes that made the process easier. Some of these are worth confirming before moving forward.

Blocking discussion points

These are changes that change or remove behavior in a way that needs to be confirmed as "okay" before merging.

  • The previous code was testing with RuleTester 6-9. This is a bit odd to implement in typescript, so only 9 was implemented. Although, pulling the plugin in locally and re-running it on the original branch with 6-9 still passes all the tests. Even if it convolutes the code, should support be re-added for 6-9?
  • All dependencies were updated to their latest version (including eslint), as to avoid running into any stale bugs. Are there (existing) backwards compatibility complications? Should the dependencies be downgraded?
  • Removed the usage of headerMatches. It was never (technically) actually used, and added to the cognitive overhead when reading the code. Although, if there's plans for future work with it, it should probably be added back. What do you want to do?
  • Removed the tests/lib/rules/output.js file. Was it being used for anything?
  • Removed the test code noStyle2 and just used noStyle1 instead since they were identical. Is that fine?
  • Migrated all the tests to use vitest instead of mocha. This is what most modern eslint plugins use, and it avoids some open issues with TS/mocha. Is that fine? Should we migrate back?
  • Removed the TODO(): test fixed comment in /staging/staging.spec.js, as there are fixed tests. Was that meant for more explicit fixed tests?

Non blocking changes

These are changes that are just format or readability changes, and don't really have any implications as far as behavior. Although, if you'd like any of these reverted- just let me know.

  • Everything migrated to TS.
  • Migrated to @typescript-eslint
  • Comments added where behavior was a bit confusing, or not as intuitive.
  • Zod used for config validation; centralizing the validation work, and providing TS safe types for it.
  • Fixed the repo level eslint config such that it actually runs.
  • Test files re-organized; with cases and results sub directories used instead of a flat directory.
  • Staging moved to the tests directory to make integration easier.
  • Templates centralized under tests/utils/templates.
  • Names added to all tests to improve eslint's error message on failure.
  • JSDocs added for a large portion of the API.
  • Added a test case for overriding the year
  • Added a test case for user provided template variables
  • Centralized all file normalization to a single function normalizeLineEndings.
  • Removed unnecessary escape sequences in the template regexp escape pattern.
  • Added a race-condition safe alternative for reading template files.
  • Added copyright headers to all the files
  • Bumped the version in package.json by a minor.
  • Added a recommended config which automatically adds the plugin.
  • Modified the plugin declaration to use the root package.json as a SOT for the name and version.

Out of scope

Work or features that may be worth doing in the future, but are out of scope for this PR.

  • Rule documentation. This doesn't need to be hosted anywhere, you could have it on GitHub directly, for example.
  • Eslint Schema. Using zod-to-json-schema this would be moderately easy to implement, but it comes with some minor nuances that make it not worth addressing in this PR.
  • A migration to ESM. That comes with its own complications, so I'll defer that to the future.

@nickdeis
Copy link
Owner

nickdeis commented Dec 6, 2024

For some of these I'll need to pull the code and test it myself, but let me just get the definite yes's out of the way.

Migrated all the tests to use vitest instead of mocha. This is what most modern eslint plugins use, and it avoids some open issues with TS/mocha. Is that fine? Should we migrate back?

That sounds awesome. I was going to use node:test but I will need to read a bit more on vitest. If does testing stuff I'm pretty much fine with whatever. Mocha is quite bad nowadays, especially for async.

Removed the TODO(): test fixed comment in /staging/staging.spec.js, as there are fixed tests. Was that meant for more explicit fixed tests?

Nope, just forgot to remove it.

The previous code was testing with RuleTester 6-9. This is a bit odd to implement in typescript, so only 9 was implemented. Although, pulling the plugin in locally and re-running it on the original branch with 6-9 still passes all the tests. Even if it convolutes the code, should support be re-added for 6-9?

I'd like to look into this more, as I just went through this with https://github.com/nickdeis/eslint-plugin-no-secrets. Let me see if I can do some npm and typescript hacking to make this a bit better.

@nickdeis nickdeis self-assigned this Dec 6, 2024
@nickdeis
Copy link
Owner

nickdeis commented Dec 6, 2024

If it's easier both of us, I could just make you a "contributor" or whatever role allows you to make MRs from my main. In the meantime I could create a branch and merge these changes in to help out.

@daymxn
Copy link
Collaborator Author

daymxn commented Dec 6, 2024

For some of these I'll need to pull the code and test it myself, but let me just get the definite yes's out of the way.

That sounds awesome. I was going to use node:test but I will need to read a bit more on vitest. If does testing stuff I'm pretty much fine with whatever. Mocha is quite bad nowadays, especially for async.

I'd like to look into this more, as I just went through this with https://github.com/nickdeis/eslint-plugin-no-secrets. Let me see if I can do some npm and typescript hacking to make this a bit better.

SGTM, I'd be interested in what you come up with!

If it's easier both of us, I could just make you a "contributor" or whatever role allows you to make MRs from my main. In the meantime I could create a branch and merge these changes in to help out.

Also SGTM, we could move it to a dev branch or ts-migration, and that'd make it easier to do additional work and run CI on it.

@nickdeis nickdeis changed the base branch from master to migrate-to-typescript December 10, 2024 02:58
@nickdeis nickdeis merged commit 9a5caa5 into nickdeis:migrate-to-typescript Dec 10, 2024
2 checks passed
@nickdeis
Copy link
Owner

Alright moved it to over here: #30 and gave you "contributor" status

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.

2 participants