[Linter] Add a linter rule to check if sdk-type exists in package.json

[bzhang0]

This fixes #13214. Let me know if there is anything I missed! This is my first time contributing to a big project (or an open source project in general)!

A couple comments + what I did:

• I implemented a new rule ts-package-json-sdktype-exists.ts that simply checks if sdk-type exists in package.json files. If it does not, then the linter will throw an error.
• I added appropriate tests and verified the entire repository using rush lint
• I also put the rules in the correct files (src/configs/index.ts, src/rules/index.ts, and tests/plugin.ts)
• In retrospect, this could probably be merged with the ts-package-json-sdktype.ts, as they seem to have the same combined functionality (one checks if it exists, the other checks if it has the proper fields)
• I have written a docs file to put in docs/rules but it seems that these directories are in the .gitignore. Let me know if this is intentional, but regardless I have the appropriate docs ready if needed.

Hopefully this is alright, thanks for your time!

[ghost]

Thank you for your contribution bzhang0! We will review the pull request and get back to you soon.

[maorleger]

Hi @bzhang0 - welcome and thank you for your contribution! ❤️

What you have here makes sense to me; however, I have a few thoughts:

I implemented a new rule ts-package-json-sdktype-exists.ts that simply checks if sdk-type exists in package.json files. If it does not, then the linter will throw an error.
I added appropriate tests and verified the entire repository using rush lint
In retrospect, this could probably be merged with the ts-package-json-sdktype.ts, as they seem to have the same combined functionality (one checks if it exists, the other checks if it has the proper fields)

I agree with you, I believe these should be merged - would you want to give that a shot? I would expect ts-package-json-sdktype to have the following rules:

1. sdk-type exists
2. sdk-type is one of the expected values ["client", "mgmt"]

Thank you for writing tests for these - I believe your tests will work for the existing rule as well.

I would recommend the following:

1. Rename tests/rules/ts-package-json-sdkytype-exists.ts -> tests/rules/ts-package-json-sdktype.ts
2. Move the logic from src/rules/ts-package-json-sdktype-exists.ts to src/rules/ts-package-json-sdktype.ts (I notice it skips any entries where sdk-type does not exist, which I believe can now be deleted since we expect it to always exist)
3. Remove ts-package-json-sdktype-exists rule
4. Try adding an invalid entry where the package.json does not include sdk-type at all

I have written a docs file to put in docs/rules but it seems that these directories are in the .gitignore. Let me know if this is intentional, but regardless I have the appropriate docs ready if needed.

I don't think that's intentional, might be an overzealous gitignore rule. (git add -f <filepath> will help you add the doc). I will look into whether we need to update the gitignore file 😄

[bzhang0]

Hi @maorleger! Super excited to join and help out!

I can definitely merge the two rules together - it's pretty clear that if sdk-type can be checked, then it must exist. I will also rename/fix relevant files appropriately, and add the docs to a new commit (I left a message on the other PR I made, #18567, about potentially making a new fork and being more consistent with my branching).

I will also follow through with your recommendations, but I had a question about 4:

Try adding an invalid entry where the package.json does not include sdk-type at all

Does this mean checking to see if the linter actually catches the error? I actually ran into this since I ran rush lint before runningrush rebuild, so it got caught on sdk/keyvault/keyvault-common/package.json, shown here: c651e91. If there was something else you meant by this, let me know :)

[maorleger]

Does this mean checking to see if the linter actually catches the error? I actually ran into this since I ran rush lint before runningrush rebuild, so it got caught on sdk/keyvault/keyvault-common/package.json, shown here: c651e91. If there was something else you meant by this, let me know :)

I just meant having a test case for it. I think you already have it tested indirectly👍

azure-sdk-for-js/common/tools/eslint-plugin-azure-sdk/tests/rules/ts-package-json-sdktype-exists.ts

Lines 374 to 382 in c651e91

	// sdk-type is in a nested object
	code: '{"outer": {"sdk-type": "client"}}',
	filename: "package.json",
	errors: [
	{
	message: "sdk-type does not exist at the outermost level"
	}
	]
	},

I'm not sure if there's a test file for ts-package-json-sdktype so you can probably rename the test file and (optionally) add a test for:

{
   // sdk-type is not one of the allowed values
   code: '{"sdk-type": "foo"}', 
   filename: "package.json", 
   errors: [ 
     { 
            ...
     } 
   ] 
 }, 

Hope that makes sense!

[bzhang0]

Understood! I'll definitely be updating the test cases after merging the two rules together :)

[bzhang0]

I will be closing this PR to re fork and make my changes in separate branches.

Be back soon! :)