[ESLint Plugin] No extraneous dependencies in imports

[deyaaeldeen]

Makes sure that we add deps for the source code in package.json's dependencies section and not in the other ones. Fixes #5564

[ghost]

Hello @deyaaeldeen!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ramya-rao-a]

Please check with @jonathandturner if this was left as dev dependency on purpose. As noted in #13103, this increases the bundle size significantly

cc @xirzec

[deyaaeldeen]

@ramya-rao-a I was curios about this as well, it looks weird to depend on express in the SDK. Either way, it is my understanding that if someone tries to use interactive browser credentials in identity now and they do not happen to have express installed, they will get an error because it is being used in the source code. So I believe this is a bug in a shipped package and have to be fixed by at least adding express as a dependency for the time being. That being said, I understand the bundle size concern and for now I would recommend to the customer to mark express as external in their webpack configs if they do not need the interactive browser creds module.

[sophiajt]

If I recall correctly, I added it so that interactive browser would work correctly. Currently, IBC isn't optional functionality, it's part of the core features of identity.

We could look into writing that part of the IBC functionality differently.

[witemple-msft]

peerDependency could be an option here.

[xirzec]

Taking a runtime dep on express for IBC feels bad. I'd like to rewrite if possible. In a past life I wrote a very small HTTP server in C# in order to do code coverage, so it's not as bad/crazy as it sounds.

[deyaaeldeen]

@ramya-rao-a if we are not adding express as a dep, I think we may as well remove IBC all together until we have a good implementation for it.

[maorleger]

is this the only usage of express for IBC? Maybe we can replace it with vanilla node counterparts instead as a quick(ish) fix (in a separate PR / issue)?

Like would it work if we replaced express with http+url (for query string parsing)?

I'm curious about longer term - Since this in the browser I think I'm missing how it works today. 😄 Edit: specifically who spins up the express server and how does it work from a browser

[xirzec]

I'm a little uncomfortable shipping a GA version of identity that depends on express, but since it's not breaking to remove a dependency, I would be fine shipping as is and refactoring this during the month so we can release a version that doesn't depend on express in March.

[ramya-rao-a]

Looks like we already shipped 1.2.3 and the next Identity release will be preview. So, lets go ahead with this and ensure we do the refactoring for IBC this month

[deyaaeldeen]

Sounds good. I will merge the PR as is. Perhaps we should consider a patch release with this PR change.

[witemple-msft]

This looks good to me. For what it's worth, I took a look at Airbnb's configuration of this same plugin, since it has thousands upon thousands of users. It's worth looking over to see if there's anything we can add to this config. For example, they include karma.conf.js in the devDependencies list.

https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/imports.js