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(linter): stabilize no-barrel-file #3069

Closed
Boshen opened this issue Apr 22, 2024 · 16 comments
Closed

feat(linter): stabilize no-barrel-file #3069

Boshen opened this issue Apr 22, 2024 · 16 comments
Assignees
Labels
C-enhancement Category - New feature or request

Comments

@Boshen
Copy link
Member

Boshen commented Apr 22, 2024

Follow up on #3030

Need to test in real projects.

Currently seeing false positives, and also panics.

@Boshen Boshen added the C-enhancement Category - New feature or request label Apr 22, 2024
@Boshen Boshen self-assigned this Apr 22, 2024
@rzvxa
Copy link
Collaborator

rzvxa commented Apr 27, 2024

I've tested it a few times and it is stable, The issue was with the root_node which is fixed now. I think we can close this one although we may want to create another issue to improve its detection and false positives, But as It is now we are doing a similar thing to biome (we are both deriving this lint rule from the same source).

@Boshen
Copy link
Member Author

Boshen commented Apr 27, 2024

Ok, I'll test again once I get the time.

@Boshen
Copy link
Member Author

Boshen commented May 1, 2024

image

Reporting 0 modules in https://github.com/mermaid-js/mermaid

@Boshen
Copy link
Member Author

Boshen commented May 1, 2024

image

False positive in kibana and vscode? no export * in sight.

@rzvxa
Copy link
Collaborator

rzvxa commented May 1, 2024

image

Reporting 0 modules in https://github.com/mermaid-js/mermaid

Have you looked at the source code for this? I think it is a problem with Resolver, This package has a lot of funky stuff, both generated codes and js extensions for ts files[when importing them].


####Edit:

for example all of these js includes are actually ts files.

@rzvxa
Copy link
Collaborator

rzvxa commented May 1, 2024

image

False positive in kibana and vscode? no export * in sight.

I believe this one would also be positive with this eslint plugin - which I've based this rule on - It doesn't exclusively look at star exports it counts the number of exports and declarations if exports are higher than declarations and also greater equal a certain threshold then it would be marked as a barrel file.

@Boshen
Copy link
Member Author

Boshen commented May 1, 2024

It doesn't exclusively look at star exports it counts the number of exports and declarations if exports are higher and also than declarations and also greater equal a certain threshold then it would be marked as a barrel file.

Oh this is counter intuitive, let me take a deeper look.

@rzvxa
Copy link
Collaborator

rzvxa commented May 1, 2024

image
Reporting 0 modules in https://github.com/mermaid-js/mermaid

Have you looked at the source code for this? I think it is a problem with Resolver, This package has a lot of funky stuff, both generated codes and js extensions for ts files[when importing them].

####Edit:

for example all of these js includes are actually ts files.

Maybe we shouldn't report a number when we can't resolve all import/exports, Is there anything in the Resolver that can check if we have unresolved import/exports?

@Boshen
Copy link
Member Author

Boshen commented May 1, 2024

Ohh ... so there's no formal definition for what a "barrel file" is ...

Definition 1:

https://gist.github.com/developit/a306951af9c0cfdf5925f126428887eb#file-no-barrel-js-L126

Using the facade value from es-module-lexer. https://www.npmjs.com/package/es-module-lexer#facade-detection

Facade modules that only use import / export syntax

Definition 2:

https://github.com/thepassle/eslint-plugin-barrel-files/blob/main/lib/rules/avoid-barrel-files.js

Number of exports is greater than number of definitions.

Biome just ported the 3 rules from eslint-plugin-barrel-files


I think for oxlint, Definition 1 is definitely better than needing to understand and apply the 3 different rules from eslint-plugin-barrel-files.

And I've already done all of the work for esmodule lexer 😁 https://github.com/oxc-project/oxc/tree/main/crates/oxc_module_lexer, I would just use it 🤣

@Boshen
Copy link
Member Author

Boshen commented May 1, 2024

Cool, found the source: import-js/eslint-plugin-import#2922 (comment)

@rzvxa
Copy link
Collaborator

rzvxa commented May 1, 2024

Well feel free to discard it in favor of detecting facades, Right now I've mostly invested my time in the transformers and traversable AST otherwise I would've offered refactoring it myself.

If it isn't too urgent I can do it when I find some free time(I should probably get done with it before #3071).

@Boshen
Copy link
Member Author

Boshen commented May 1, 2024

There is no rush! Sorry I didn't invest enough time to understand the full context, twitter sometimes gets into me and I just rushed things :-(

@rzvxa
Copy link
Collaborator

rzvxa commented May 4, 2024

@Boshen
Now that we almost have a working solution for the traverse, I'd like to revisit this rule.
Should I discard it in favor of detecting facade modules?

The number of modules reported in the mermaid has nothing to do with this rule. So even if we go for the use of oxc_module_lexer it won't change that. There might be a build step missing while you've been testing it since the code is importing non-existing modules.

@Boshen
Copy link
Member Author

Boshen commented May 4, 2024

Should I discard it in favor of detecting facade modules?

I think so, I want to have Shopify use more oxlint rules so it can eventually drop eslint for all custom rules ;-)

After reading relevant issues in the eslint-import-plugin repo, I think facade should be the more appropriate method to use.

the number of modules reported in the mermaid has nothing to do with this rule.

understood 😁

@Boshen
Copy link
Member Author

Boshen commented May 14, 2024

Found a better representation https://twitter.com/boshen_c/status/1790353605234655637

image

@rzvxa
Copy link
Collaborator

rzvxa commented May 14, 2024

Found a better representation https://twitter.com/boshen_c/status/1790353605234655637

image

I saw it in Discord, It is so nice😍

Boshen pushed a commit that referenced this issue May 15, 2024
Boshen pushed a commit that referenced this issue May 15, 2024
Boshen pushed a commit that referenced this issue May 16, 2024
@Boshen Boshen closed this as completed May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants