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

chore: remove redundant assignments & use "createRequire" #437

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

Foxeye-Rinx
Copy link
Contributor

@Foxeye-Rinx Foxeye-Rinx commented Oct 25, 2024

Redundant Assignments:

These lines are no longer needed because these fields are added by createRule function when the rules are initialized

- rule.meta.docs.ruleName = ruleName
- rule.meta.docs.ruleId = ruleId

Make it more ES module by using createRequire:

Points to discuss:

Dear @ota-meshi, I think it's better to abandon this file and create an index file in the src/rules module:

  • To take a step forward in making this package compatible with Bun/Deno

  • ESModule import loads modules asynchronously behind the scenes, so it’s faster than using this module loader, the rules files will be loaded in parallel by the runtime (Ref)

  • The new index file is more familiar to developers who are not from the CommonJS era → Welcoming new contributors

  • The src/rules files export as ES modules, so importing them as ES modules is ideal

    If you agree with these points, I’ll close this and create a new PR for the index file. You can preview the commit for the index file at my fork here

Redundant Assignments:

These lines are no longer needed because these fields are added by `createRule` function when the rules are initialized
```
rule.meta.docs.ruleName = ruleName
rule.meta.docs.ruleId = ruleId
```
Make it more ES module by using `createRequire`:

- No longer suppress the ESLint, the ESLint was warned for using `require` in a ts file. [@typescript-eslint/no-require-imports](https://typescript-eslint.io/rules/no-require-imports)
Copy link

changeset-bot bot commented Oct 25, 2024

⚠️ No Changeset found

Latest commit: 2db3794

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Owner

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ota-meshi ota-meshi merged commit fd8bc14 into ota-meshi:main Oct 25, 2024
8 checks passed
@ota-meshi
Copy link
Owner

I think it's better to abandon this file and create an index file in the src/rules module:

I don't quite understand it. What is the difference between src/utils/rules.ts and src/rules? Can you explain in more detail?

https://github.com/ota-meshi/eslint-plugin-astro/blob/66f55f8e1a168bf16c938f2b15efd3b2c383a0e5/src/utils/rules.ts

@Foxeye-Rinx
Copy link
Contributor Author

Foxeye-Rinx commented Oct 25, 2024

Ah yes, let me clear this out:

  • The tools/lib/load-rules.ts is just to export the rules array of all rules in /rules
  • The new rules/index.ts file does the same as above
  • That's why I want to remove the load-rules.ts and replace it with the new rules/index.ts
  • Now you'll see that the new rules/index.ts file is almost the same as src/utils/rules.ts
  • The only difference is that the src/utils/rules.ts exports the a11y's rules, while the rules/index.ts does not
  • 👆 to solve that kind of duplication I think we can just remove the src/utils/rules.ts and its generator - tools/update-rules.ts and then import the a11y's rules to the new index.

👉 These changes will remove a lot of complexity and simplify this package.

@ota-meshi
Copy link
Owner

There's no problem with adding src/rules, but I don't think we can remove load-rules.ts.
If we want src/rules/index.ts to be generated by npm run update, we'll need to load rule information from another.

@Foxeye-Rinx
Copy link
Contributor Author

Foxeye-Rinx commented Oct 26, 2024

Dear @ota-meshi, it's removable, to illustrate this better, I just created #438, the load-rules.ts and the utils/rules.ts are now kind of duplication. Unifying them to the new index file also allows us to remove the generator tools/update-rules.ts and makes it simpler.
The rules files located in src/rules are currently managed manually (not using generator), and the new index should also be managed manually.

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