Add eslint-plugin-fluid package in common/build

[jikim-msft]

Description

Follow up to:

• Add eslint-custom-rules package to eslint-config-fluid #19217
• Revert "Add eslint-custom-rules package to eslint-config-fluid (#19217)" #19549

Instead of creating a nested package inside the eslint-config-fluid package, this PR creates a new package to enforce custom linter rules.

[jikim-msft]

Not sure why this is automatically chaning from ts to js just by moving the directory.

[alexvy86]

In general imports shouldn't have ts extensions. js/cjs/mjs are ok. I imagine you probably told VSCode to auto-update imports when the file moved, and if so it did the right thing here.

[jikim-msft]

FluidFramework/common/build/eslint-config-fluid/src/test/mockFiles/no-restricted-tags-imports/fileWithExceptionImports.ts

Line 6 in 22b05b0

import { exceptionInternalFunction, alphaFunction2 } from "./exceptionFile.ts";

I think using .ts extension is acceptable as this was the case from the eslint-config-fluid package?

[alexvy86]

Not really. It doesn't fail because we're not actually running those files, just linting them, but Typescript is complaining about the import even now in my local:

Best to just use .js even if these files are never really executed.

[jikim-msft]

Left comment below about changing it to .ts.

[alexvy86]

Just importing with a .js extension causes the test to fail because there's no such .js file. Note this project doesn't do a Typescript build. The actual tests are written in .js files (src/test/enforce.../*.js), and all these .ts files inside src/test/mockFiles/ are just collateral for testing the custom linting rules, and they're never compiled. It feels right to me that the rules are tested "on top of" .ts files since that's what they would normally operate on in the wild.

For the purpose of the test, we just need that one of the .ts files is importing from another one that exports things with different API-levels, and I guess writing both in TS was the more natural thing to do. If exceptionFile was a .(c/m)js file directly and we imported from it with that extension, things probably would be fine too? We'd just need to remove the typing from it. Not sure I prefer that so much over the current state of things, maybe expanding on the comment in line 6 about why we do it.

I imagine a setup exists where we compile the collateral files during the build, so then when the tests for the custom lint rules run they have both the source and the built versions, but I'm not sure what exactly we gain from such a setup.

[jason-ha]

There is a test tsconfig.json - not part of the normal build, but it will be.
Of course there isn't a .js file, but .js is the way import specs should be written ... without allowImportingTsExtensions. Typescript and related evaluators should understand that a .js path may resolve to .ts or .d.ts in a Typescript context. There is no requirement that the .js file exists during Typescript linting. (Our build used to require tsc run before eslint, but that was wrong - I think I removed that requirement. If not, I will get it removed ;) )
Note that we do not use allowImportingTsExtensions in our normal tsconfig settings. The settings here should be similar to the minimal requirements for our build. (So, no allowImportingTsExtensions.)
Technically we could lint a .js file for the tags associated with the imports by looking at the corresponding .d.ts, but I think that is uncommon use case and we can ignore it.
I am concerned that the related rules are not really doing their job in the real world.

[alexvy86]

So the issue here might be that the "test" tsconfig.json should look more like one of our normal tsconfig.json's, and that might let the dummy TS project which gets linted in this package's tests to import from ... .js" like we do everywhere else?

Of note, I hesitate to just call it "test tsconfig.json" because that makes me think of "the tsconfig json used for the build of the tests", which at least today, this one is not; neither source nor tests have a build step in this package right now. The tsconfig.json is used for a dummy TS project "created programmatically" for the ESLint instance we create and exercise during tests.

I am concerned that the related rules are not really doing their job in the real world.

no-member-release-tags does trigger today:

no-restricted-tags-imports... seems not to be used today, ha.

[jason-ha]

Yeah - it should be the "examples" tsconfig file. (I didn't go as far as to suggest relocating the file in #22005, but we definitely should when making this package use typescript.)

[jason-ha]

I am not saying no-member-release-tags won't trigger today. I am questioning whether it is triggering everywhere we want it to. I have not looked at the rule to try to understand it.

[alexvy86]

A few comments

[msfluid-bot]

⯅ @fluid-example/bundle-size-tests: +187 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	511.94 KB	511.98 KB	⯅ +44 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	244.62 KB	244.64 KB	⯅ +22 Bytes
loader.js	170.65 KB	170.67 KB	⯅ +22 Bytes
map.js	46.53 KB	46.54 KB	⯅ +11 Bytes
matrix.js	148.88 KB	148.9 KB	⯅ +11 Bytes
odspDriver.js	97.67 KB	97.7 KB	⯅ +33 Bytes
odspPrefetchSnapshot.js	42.63 KB	42.65 KB	⯅ +22 Bytes
sharedString.js	168.61 KB	168.62 KB	⯅ +11 Bytes
sharedTree.js	333.89 KB	333.89 KB	■ No change
Total Size	1.87 MB	1.87 MB	⯅ +187 Bytes

---

Baseline commit: ecb68d5

Generated by 🚫 dangerJS against 441cfee

[alexvy86]

Lgtm! One nit below.

[Josmithr]

I don't think these are right. This package doesn't have a package-lock.json, or printed-configs or type-tests. It might be better to remove this file and specify the .prettierignore at the repo root instead in the prettier package.json scripts.

[jikim-msft]

You're referring to this .prettierignore file located in the root of the client release group right?

FluidFramework/.prettierignore

Line 1 in 32cfa09

# Generated by npm / pnpm

eslint-plugin-fluid generates nyc and node_modules and these are already covered in the file, so I believe we wouldn't need any more addition.

[Josmithr]

Left a handful of suggestions, but otherwise looks good to me! Thanks for doing this!!

[jikim-msft]

@alexvy86 @Josmithr

Wanted to double check if removing taskTest is correct as the test-suites in eslint-config-fluid have been moved to the eslint-plugin-fluid package.

[Josmithr]

It is correct, though I would recommend an alternative solution. I would recommend keeping this, and replacing the test script in the package with echo "TODO: add tests". That way if we add new tests later, the CI pipeline will automatically run them.

The reason I suggest this is that when we first added tests to the package a few months ago, none of us realized we needed to modify the pipeline to ensure they were running. So the tests weren't actually being run until I happened to notice that one of them was failing locally and did some digging.

[alexvy86]

I'll just add it's actually not entirely correct 😄 , it needs to be specified as taskTest: [] otherwise the default will take over. Example where we disable them here. But I agree with Josh's suggestion to just add a test script to the package, and keep tasktest: \n - test in the pipeline.

[Josmithr]

Looks great! I left one more suggestion, but otherwise looks good to go! 🚀

[alexvy86]

I'll just add it's actually not entirely correct 😄 , it needs to be specified as taskTest: [] otherwise the default will take over. Example where we disable them here. But I agree with Josh's suggestion to just add a test script to the package, and keep tasktest: \n - test in the pipeline.