A-Shleifman / eslint-plugin-export-scope

Disallows importing scoped exports outside their scope
https://www.npmjs.com/package/eslint-plugin-export-scope
104 stars 0 forks source link

False positives for imports from another package #9

Open sergei-dyshel opened 2 weeks ago

sergei-dyshel commented 2 weeks ago

I have an NPM workspace with multiple packages, say @sergei-dyshel/typescript and @sergei-dyshel/node. I see that ESlint plugin triggers warning:

.../packages/qcfg-js-node/src/lib/cache.ts
  1:10  warning  Cannot import 'jsonStableStringify' outside its export scope  export-scope/no-imports-outside-export-scope

while this symbol is imported from second package:

import { jsonStableStringify } from "@sergei-dyshel/typescript/json";
A-Shleifman commented 2 weeks ago

This plugin doesn't differentiate between normal paths and package paths. From the perspective of the plugin you're trying to import from @sergei-dyshel/typescript/json which I assume is the same as ../node/json which is outside of the scope by default.

So you have the standard options:

  1. declare a wider scope for the json file. (maybe set the scope to .. in @sergei-dyshel/typescript/.scope.ts)
  2. keep the scope, but set @scopeException to only allow @sergei-dyshel/node to import from the typescript package.
  3. you can always ignore the eslint rule for a one-off import

Please let me know if you feel this is not sufficient and a direct support for monorepo packages is required. I just think that the more scenarios can be resolved with generic solutions the better.

sergei-dyshel commented 2 weeks ago

Hey Alex, thanks for quick response! Defining per-file or per-directory scopes is not appropriate, as I have many files/directories and imports from them. I'd like to use something like strictMode: false from previous version. I tried putting .scope.ts with export default "*" in the root and src folders in packages, but it didn't work. Any insights?

A-Shleifman commented 2 weeks ago

Would adding support for a new property inside the .scope.ts files solve the problem?

// @/typescript/json/.scope.ts

export default '../..'; (or); export const exceptions = ['../../node']

export propagate = true;
// 👆 This would make even nested exports from `json` folder to be accessible from `@`

I just need to think about the performance of this solution and ergonomics.

/** @propagate */
export default '../..';; 

export const exceptions = []

maybe this is better as it allows propagation of only some of the settings, but it introduces another annotation that only works in .scope.ts files.

sergei-dyshel commented 2 weeks ago

Why is new property needed? Isn't there an existing way to mark all exports in the package as importable for everyone?

A-Shleifman commented 2 weeks ago

.scope.ts files only change the scope of exports declared in that folder. Any nested folders will have their own scopes and will ignore all parent .scope.ts files.

So with this new property, you would be able to make everything under @sergei-dyshel/typescript (including subdirs, that's what you want, right?) accessible from @sergei-dyshel/node without making '*' the default scope for every single export in the project (which provides less safety).

Am I missing something?

sergei-dyshel commented 2 weeks ago

Hmm, I supposed that .scope.ts put in the root folder would achieve that. According to README:

Root .scope.ts file (next to package.json) sets the default for the whole project. Having export default '*' there will make all exports global by default if you prefer a less strict approach.

Or does it mean that exports will be visible to that package only, but not to other packages? If so, you need to change that behaviour because export visibility between packages should be controlled by package.json only and not affected by the plugin.

A-Shleifman commented 1 week ago

It looks like I missed a test for this feature and it doesn't work in v2. Saying that I'm not sure I want it to work this way as:

  1. having .scope.ts apply only to the current directory everywhere in the project apart from the root folder is inconsistent and might be confusing.
  2. this feature lacks granularity. If there's only one folder in the project that needs to have all exports exposed, the only way to do it would be to change the default export scope for all exports in the project which partially defeats the purpose of this plugin.

A more granular solution that allows relaxing the scope only for some subdirs might be better.

Option 1: const propagate = true;

// .scope.ts
export default ''; 
export const exceptions = [...]

export propagate = true; // 👈 this would change the scope for all subfolders in addition to the current folder

A problem: if the scope can be inherited from any of the parent .scope.ts files, keeping track of the current scope might be harder for devs.

Option 2: .children.scope.ts

Having a separate file for setting a new default scope for children might make it easier to keep track of the current scope.

// .children.scope.ts
export default ''; 
export const exceptions = [...]

What do you think?

sergei-dyshel commented 1 week ago

I vote for propagate option because using separate file for children seems overkill....

TBH I don't understand why you went with .scope.ts files in the first place instead of plain JSON files (with defined schema).

A-Shleifman commented 1 week ago

Thank you for the feedback!

The JSON file would have to be manually updated every time folders/files are moved within the project which would make refactoring very annoying. .scope.ts files are relative, so they can easily move with the folder.