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

Feat/all-technologies-config #101

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Charlie-Crowdstrike
Copy link
Contributor

We can make this repo the one stop shop for all crowdstrike configs.

node ruleset is taken from https://github.com/CrowdStrike/eslint-config-crowdstrike-node

@@ -1,6 +1,6 @@
'use strict';

module.exports = {
export default {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

turning these into ESM syntax

import javascript from "./javascript.js";

export default [
...javascript,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want to include the javascript rules here by default, or allow developers to mix in the inheritance themselves?

import js from '@eslint/js';

export default [
js.configs.recommended,
Copy link
Contributor Author

@Charlie-Crowdstrike Charlie-Crowdstrike Sep 16, 2024

Choose a reason for hiding this comment

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

should we bake these in these js.configs.recommended, defaults? or let developers choose if they want to include

nodePlugin.configs['flat/recommended'],
...javascript,
{
files: ['**/*.json'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wondering if we should break these out into a separate json rulset, and if so, if we should include it in this ruleset definition or leave it separate for developers to mix in as they choose?

'yield-star-spacing': 'error',
},
}];
export { ember, javascript, node, typescript };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would need a major version release, as this breaks how its imported in existing versions

@@ -2,10 +2,8 @@
"name": "eslint-config-crowdstrike",
"version": "11.0.1",
"description": "ESLint config for CrowdStrike",
"main": "index.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing this package to module. Would definitely need to be released as a major version as this is a breaking change

@@ -33,13 +31,16 @@
},
"homepage": "https://github.com/CrowdStrike/eslint-config-crowdstrike#readme",
"dependencies": {
"@eslint/js": "^9.0.0"
"@eslint/js": "^9.10.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open question is if we should keep this as a single bundle and assume the consuming project will treeshake the dependencies on build, or if we should have individual package.json's for each ruleset, and be a true monorepo

@Charlie-Crowdstrike
Copy link
Contributor Author

I def wanna release this as a beta/release-candidate first so i can test it in other repo's first

{
files: ["**/*.{js,mjs,cjs,jsx,gjs}", "tests/**/*.*ts"],
// These rules don't apply to JavaScript. And for tests they're annoying
// due to how much of "@open-wc/testing" is typed as `any` or inconsistently.

Choose a reason for hiding this comment

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

I wonder if we should remove the part about about the tests/**/*.*ts directory and@open-wc/testing comment here, since it is pretty specific to that one library.

I feel like in general we wouldn't want to disable typescriptEslint.configs.disableTypeChecked for anything inside of a tests/**/*.*ts directory. If those are also TS files, then it seems like something we should lint as typescript unless there's a good reason not to. Also different projects may have different a structure for where the tests could be.

export default [
...javascript,
...typescriptEslint.configs.recommendedTypeChecked,
...typescriptEslint.configs.stylisticTypeChecked,

Choose a reason for hiding this comment

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

I'm not sure if all projects would want these rules from stylisticTypeChecked by default or not. I could imagine that some projects might use Prettier directly, or have some other specific ruleset for their formatting.

What do you think?

@Charlie-Crowdstrike Charlie-Crowdstrike changed the title Feat/monorepo Feat/all-technologies-config Sep 25, 2024
@Charlie-Crowdstrike
Copy link
Contributor Author

V2 can be separate package.json's / true monorepo

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