Enforce no export * in eslint

[ghoshkaj]

As part of converting all the export *'s to named exports, we need to enforce that future PRs cannot introduce any export *s. This PR introduces an eslint rule that will enforce this.

Related Issue: #10062

[alexvy86]

LGTM, but I'd still let Tyler chime in when he has a chance.

And made me curious if there's a reason we still have the eslint7.js file when the only place it seems to be used/extended (in our repo, at least) is minimal.js, so I'm tempted to merge them. But we'd need to know if an internal partner is using it.

[tylerbutler]

And made me curious if there's a reason we still have the eslint7.js file when the only place it seems to be used/extended (in our repo, at least) is minimal.js, so I'm tempted to merge them. But we'd need to know if an internal partner is using it.

There's no need for the eslint7.js file anymore. It was used when we migrated to eslint8 awhile back, to make it easier to gradually upgrade. My (quite old) PR #9748 is the ultimate direction we've been going with the config, and you'll note that the eslint7.js file is gone in that PR.

I don't think we'll merge that PR in as-is, but it represents the direction we've been heading. @Josmithr may have more to add, but I think it's OK to leave as-is for now.

[Josmithr]

I would recommend enabling this rule in our strict config, rather than in the base one(s). I'm not sure every package will necessarily want this rule. Then we can update the packages we want to prioritize to use the strict config on a package-by-package basis.

For what it's worth, I think using our strict api-extractor config, which enforces that anything transitively exposed in the public API is explicitly enumerated in the root package export, gets us the same sort of enforcement we're looking for here without needing to forbid export *. This only works for packages using API-Extractor, but I think all of our library packages should be using that anyways.

I guess my other concern with forbidding this pattern wholesale is that it removes the ability to use export * in package sub-directories. I'm a big fan of explicitly enumerating the API contents in the package root export, but it is often nice to be able to export * the contents of package sub-directories for use by other package internals (if that makes sense). If we could enforce this only for the package root export module, that would be fantastic.

[tylerbutler]

I would recommend enabling this rule in our strict config, rather than in the base one(s). I'm not sure every package will necessarily want this rule. Then we can update the packages we want to prioritize to use the strict config on a package-by-package basis.

I think @ghoshkaj has gone to great lengths to make it possible for us to enable this rule across the repo, so I'd rather get this into the minimal config.

I guess my other concern with forbidding this pattern wholesale is that it removes the ability to use export * in package sub-directories.

I think we should be treating subdirectories as modules with an explicit API. There was a discussion about this recently; I'll find a link. From that discussion, @ChumpChief pointed out:

The main problem I've hit with subdirectory organization is that it can cause tension with the no-internal-modules rule. a naive response to that lint error might be to over-export subdirectory contents, in the worst case unintentionally exporting internal concepts across the package boundary if they're not careful.

And @CraigMacomber mentioned:

packages/dds/tree has lots of directories under src, each with their own index. I like this pattern as it allows for more scope control. We also use good-fences to control deps between the folders. We just suppress the import restriction for the specific file under test, and leave it on for everything else.

This is also the pattern I adopted in build-cli, sans the readme (which I should add).

[Josmithr]

I would recommend enabling this rule in our strict config, rather than in the base one(s). I'm not sure every package will necessarily want this rule. Then we can update the packages we want to prioritize to use the strict config on a package-by-package basis.

I think @ghoshkaj has gone to great lengths to make it possible for us to enable this rule across the repo, so I'd rather get this into the minimal config.

I guess my other concern with forbidding this pattern wholesale is that it removes the ability to use export * in package sub-directories.

I think we should be treating subdirectories as modules with an explicit API. There was a discussion about this recently; I'll find a link. From that discussion, @ChumpChief pointed out:

The main problem I've hit with subdirectory organization is that it can cause tension with the no-internal-modules rule. a naive response to that lint error might be to over-export subdirectory contents, in the worst case unintentionally exporting internal concepts across the package boundary if they're not careful.

And @CraigMacomber mentioned:

packages/dds/tree has lots of directories under src, each with their own index. I like this pattern as it allows for more scope control. We also use good-fences to control deps between the folders. We just suppress the import restriction for the specific file under test, and leave it on for everything else.

This is also the pattern I adopted in build-cli, sans the readme (which I should add).

Ah, okay. If we're in good shape to enable globally, then I'm more comfortable with this. I think we might end up with some tool-wise enforcement overlap between eslint, api-extractor, good-fences, etc., but we can always re-evaluate / loosen tooling requirements later as we want. If this gets us closer to better API management, then I love it.

[tylerbutler]

If we could enforce this only for the package root export module, that would be fantastic.

An override that filtered just the root index.ts and applied the rule only to that should accomplish what you're describing. We do something similar for test files:

FluidFramework/common/build/eslint-config-fluid/minimal.js

Lines 288 to 304 in e9155b8

	{
	// Rules only for test files
	files: ["*.spec.ts", "src/test/**"],
	rules: {
	"@typescript-eslint/no-invalid-this": "off",
	"@typescript-eslint/unbound-method": "off", // This rule has false positives in many of our test projects.
	},
	},
	{
	// Rules only for type validation files
	files: ["**/types/*validate*Previous.ts"],
	rules: {
	"@typescript-eslint/comma-spacing": "off",
	"@typescript-eslint/consistent-type-imports": "off",
	"max-lines": "off",
	},
	},

[Josmithr]

If we could enforce this only for the package root export module, that would be fantastic.

An override that filtered just the root index.ts and applied the rule only to that should accomplish what you're describing. We do something similar for test files:

FluidFramework/common/build/eslint-config-fluid/minimal.js

Lines 288 to 304 in e9155b8

	{
	// Rules only for test files
	files: ["*.spec.ts", "src/test/**"],
	rules: {
	"@typescript-eslint/no-invalid-this": "off",
	"@typescript-eslint/unbound-method": "off", // This rule has false positives in many of our test projects.
	},
	},
	{
	// Rules only for type validation files
	files: ["**/types/*validate*Previous.ts"],
	rules: {
	"@typescript-eslint/comma-spacing": "off",
	"@typescript-eslint/consistent-type-imports": "off",
	"max-lines": "off",
	},
	},

That would match my personal preferences nicely and would let us further discuss how we want to manage sub-package hierarchies going forward, while still giving us more granular package API control.

But I'm also happy with this as is - we can always loosen these requirements at a later date if we decide we would prefer not to enforce in package sub-directories. Better control / awareness of our package API surfaces = absolute win as far as I'm concerned 🙂