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(noExportedImports): add lint rule #3097

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Jun 6, 2024

Summary

Add a new exclusive rule for Biome.
The rule disallows exporting an imported variable and suggests using export from.
This is an implementation I had done some time ago. I had intended to add a code fix before submitting a PR.
However, I now think that the rule is ok without a code ix.

Test Plan

I added some tests.

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis A-Changelog Area: changelog labels Jun 6, 2024
Copy link
Contributor

github-actions bot commented Jun 6, 2024

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 50227 50227 0
Passed 49120 49120 0
Failed 1107 1107 0
Panics 0 0 0
Coverage 97.80% 97.80% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6534 6534 0
Passed 2084 2084 0
Failed 4450 4450 0
Panics 0 0 0
Coverage 31.89% 31.89% 0.00%

ts/babel

Test result main count This PR count Difference
Total 669 669 0
Passed 597 597 0
Failed 72 72 0
Panics 0 0 0
Coverage 89.24% 89.24% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18244 18244 0
Passed 13981 13981 0
Failed 4263 4263 0
Panics 0 0 0
Coverage 76.63% 76.63% 0.00%

@Conaclos Conaclos force-pushed the conaclos/noExportedImports branch from 819ff72 to bdcbda9 Compare June 6, 2024 14:39
Copy link

codspeed-hq bot commented Jun 6, 2024

CodSpeed Performance Report

Merging #3097 will not alter performance

Comparing conaclos/noExportedImports (413e850) with main (b3d8bfd)

Summary

✅ 90 untouched benchmarks

@Conaclos Conaclos force-pushed the conaclos/noExportedImports branch 3 times, most recently from ce9d922 to 83565f9 Compare June 6, 2024 15:59
@Conaclos Conaclos requested a review from a team June 6, 2024 21:02
Copy link
Contributor

@ah-yu ah-yu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@Sec-ant
Copy link
Member

Sec-ant commented Jun 7, 2024

I have some use cases like this:

import { A } from "mod";
// do something with A
export { A };

Does this rule allow it? What if I mutate A and export it, should I use an alias?

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this rule go in conflict with the barrel file rule?

Comment on lines +77 to +81
"An import should not be exported. Use "<Emphasis>"export from"</Emphasis>"instead."
},
)
.note(markup! {
<Emphasis>"export from"</Emphasis>" makes it clearer that the intention is to re-export a variable."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Based on our rule pillars, we usually do: error, explain the error, suggest a fix

You should swap the notes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swap the notes?

The first diagnostic is the error and the suggestion. The second is the explanation.

In an IDE settings it is sometimes confusing to only have the error without any suggestion or explanation.

@Conaclos
Copy link
Member Author

Conaclos commented Jun 7, 2024

Would this rule go in conflict with the barrel file rule?

I don't think so.
The only thing that can happen is to suggest using export from and the nonce changed this can be reported by the noReexport rule. And this is good because re-exporting an import is a re-export.

@Conaclos
Copy link
Member Author

Conaclos commented Jun 7, 2024

I have some use cases like this:

import { A } from "mod";
// do something with A
export { A };

Does this rule allow it? What if I mutate A and export it, should I use an alias?

Have you a more precise example?
Usually imports are considered immutable?

@Sec-ant
Copy link
Member

Sec-ant commented Jun 7, 2024

Have you a more precise example?
Usually imports are considered immutable?

I was thinking something like this:

a.js:

export const user = {
	name: undefined,
};

b.js:

import { user } from "./a.js";

user.name = "John";

export { user };

index.js:

import { user } from "./b.js";

console.log(user);

But it seems I can use export { user } from "./a.js" in b.js without any issue. So I think I have no more questions.

@Conaclos
Copy link
Member Author

Conaclos commented Jun 7, 2024

@Sec-ant

Yes you can write:

import { user } from "./a.js";
user.name = "John";

export { user } from "./a.js";

@ematipico
Copy link
Member

Would this rule go in conflict with the barrel file rule?

I don't think so.
The only thing that can happen is to suggest using export from and the nonce changed this can be reported by the noReexport rule. And this is good because re-exporting an import is a re-export.

Sorry I didn't understand very well. I understood that the suggested fix could trigger noReexport?

@Conaclos
Copy link
Member Author

Conaclos commented Jun 7, 2024

Sorry I didn't understand very well. I understood that the suggested fix could trigger noReexport?

Yes, this is expected.

@Conaclos Conaclos merged commit 6f9c938 into main Jun 25, 2024
16 checks passed
@Conaclos Conaclos deleted the conaclos/noExportedImports branch June 25, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Parser Area: parser A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants