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

deprecation warnings from barrel #2

Open
snebjorn opened this issue Apr 14, 2020 · 5 comments
Open

deprecation warnings from barrel #2

snebjorn opened this issue Apr 14, 2020 · 5 comments

Comments

@snebjorn
Copy link

snebjorn commented Apr 14, 2020

Hi, with the following setting

"deprecation/deprecation": "warn"

I get warnings from my barrel (index.ts)

Like this

~\Repos\akita\libs\akita\src\lib\index.ts
  4:10  warning  'arrayAdd' is deprecated.      deprecation/deprecation

But it's not using anything, it's exporting

export { arrayAdd } from './arrayAdd';

Another thing. It's warning about functions from my own source files that have been marked as @deprecated. Perhaps that could be an option if it should do that or not.

export function arrayAdd<Root extends any[], Entity = any>(keyOrRoot: Root, newEntity: OrArray<Root[0]>, options?: AddEntitiesOptions): Root[0][];
/**
 * @deprecated
 */
export function arrayAdd<Root, Entity = any>(keyOrRoot: ArrayProperties<Root>, newEntity: OrArray<Entity>, options?: AddEntitiesOptions): (state: Root) => Root;
export function arrayAdd<Root, Entity = any>(keyOrRoot: ArrayProperties<Root> | Root, newEntity: OrArray<Entity>, options: AddEntitiesOptions = {}) {
  ...
}
@gund
Copy link
Owner

gund commented Apr 15, 2020

Hey, thanks for reporting the issue!

To be honest - I'm not the author of this plugin's code and neither spent any significant time to understand it yet.
As stated in readme - it's a copy-paste of the internal rule from sonarjs repo because they do not export it directly (you might wanna support the request to publish this rule directly from sonarjs).

So for now I would not be able to do any adjustments to this rule. However I might look into it some time later but not very soon anyway.

Also you are very welcome to try and adjust it yourself if you like and have time =)


Regarding your points.

I agree with your second point that it should not complain about deprecated things that are marked as deprecated, however if you are using them even withing your own code-base I believe it should still report it as that is old API no matter where it's used.

And about first point - I think that even re-exporting code is still considered a usage, because if that code is removed - your code will simply break. So I think that it should still report even re-exports.

@snebjorn
Copy link
Author

Ah that's good to know. I'll see what I can do about it :)

Fair points. But tests import deprecated own-code-functions until they're removed :)
The barrels simply re-export own-code-functions, and if that function is part of the public api we still have to export it until the library deprecation policy allows for its removal. So I don't consider it a mistake.

I usually go for a clean lint output as even warnings are a slippery slope. Also if we don't do anything about the warnings why are they on in the first place :) So that's my reasoning

@gund
Copy link
Owner

gund commented Apr 15, 2020

I can partially agree - when you reference/re-export your own deprecated code then yes you might want to remove those from checks as you control this piece of code.

But when you reference/re-export someone else's deprecated code then this is potentially dangerous as you do not control when that code will be removed - so I think it should be reported as the whole point of this plugin is to find and report all places where you rely on deprecated (3rd party) code so you are prepared for next major for example.

But detecting internal/external code boundaries is tricky:

  • as you do not always import from relative paths (ex. aliased imports - but still internal)
  • also you might re-export some external code internally and then import it somewhere else and it will look like it's internal import while it's not

Also there is a use-case when you develop in monorepo - then suddenly your internal code is not on the same level as you might have different packages that live together but use each other as separate/external packages. And now it's even trickier to decide what to report and what not =)

EDIT: BTW if you are testing your deprecated code - you may simply disable this rule in that test case file

@snebjorn
Copy link
Author

This is very insightful :)
It appears this issue is a lot more complex then I initially thought

@gund
Copy link
Owner

gund commented Jun 3, 2020

This issues might be partially fixed by #6 so you might give it a shot in new release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants