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

package:analyzer has too many dependencies #35802

Open
Hixie opened this issue Jan 29, 2019 · 11 comments
Open

package:analyzer has too many dependencies #35802

Hixie opened this issue Jan 29, 2019 · 11 comments
Labels
analyzer-package Issues related to package:analyzer area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Jan 29, 2019

Flutter sometimes finds it wants to depend on a package that depends on package:analyzer, and that forces all Flutter apps to take on a dependency on a ton of other packages like csslib. There's no reason why Flutter needs to depend on csslib. Unfortunately, since Flutter has to pin every dependency, we end up pinning csslib, which means that if any customer depends on csslib, they are forced to use the version we want to use, even if they want to use another version.

So far we've managed to avoid this by finding ever-more-creative ways to avoid depending on package:analyzer, but it would be good if this wasn't an issue in the first place.

@kevmoo kevmoo added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jan 29, 2019
@kevmoo
Copy link
Member

kevmoo commented Jan 29, 2019

Flutter has to pin every dependency

"Flutter has decided to pin every dependency" – for reasons that have trade-offs. Not pinning transitive dependencies eliminates this issue, but it's possible for users to get versions of packages that have not been validated.

@kevmoo kevmoo added the analyzer-package Issues related to package:analyzer label Jan 29, 2019
@stereotype441 stereotype441 added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug labels Jan 30, 2019
@kevmoo
Copy link
Member

kevmoo commented Jan 30, 2019

FYI: after https://dart-review.googlesource.com/c/sdk/+/91701 and removing the task model it seems all dependencies on pkg:html go away. And along with it pkg:css, pkg:logging and pkg:utf!

@Hixie
Copy link
Contributor Author

Hixie commented Jan 31, 2019

Nice!

(We have to pin every dependency because we're an SDK. If any of our dependencies change in a way that breaks us, it can result in our users being permanently unable to use Flutter, including upgrading. That's existential, so while it's technically a tradeoff, it's not really one we have any choice about.)

dart-bot pushed a commit that referenced this issue Jan 31, 2019
Reduces the API surface of pkg:analyzer that requires pkg:html

Related to #35802

Change-Id: Icd08d76190d6ab77cd180561cdde6df254b22557
Reviewed-on: https://dart-review.googlesource.com/c/91701
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Kevin Moore <kevmoo@google.com>
dart-bot pushed a commit that referenced this issue Mar 5, 2019
Related to #35802

R=brianwilkerson@google.com

Change-Id: I875a8dc657900137c5a7b59f2820b30a81b23405
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/95480
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@kevmoo
Copy link
Member

kevmoo commented Mar 15, 2019

Screen Shot 2019-03-15 at 3 47 58 PM

With analyzer 0.35.4 just released @Hixie !

@Stargator
Copy link
Contributor

Hmm, I saw the recent change and I was hoping to see more of a discussion on how this issue resulted in the dropping of html and plugin packages from analyzer. The complaint was about csslib, which is a dependency of html.

It seems, from a quick glance at the changes, that logic was altogether dropped instead of leverage either the native Dart SDK or some other existing dependency.

I can't tell if the logic that was dropped was deemed no longer useful or if it's somehow handled elsewhere, which is possible as I haven't gone through all the changes yet. I just need some human logic.

@kevmoo
Copy link
Member

kevmoo commented Mar 17, 2019

The pkg:html dependency was there to support parsing Dart code embedded in an HTML document in a <script> tag. This is not a scenario we care about any more. Everything else should work fine.

Oh, and pkg:html depended on pkg:css which depended on pkg:utf – so nixing one got rid of the other two.

@kevmoo
Copy link
Member

kevmoo commented Apr 5, 2019

😢 0e7bec7

pkg:html is back in pkg:analyzer

kevmoo added a commit to dart-lang/csslib that referenced this issue Apr 8, 2019
dart-bot pushed a commit that referenced this issue Apr 9, 2019
Remove use and DEPS entry for pkg:utf

Related to #35802

Change-Id: I025cbe15fc4dd7a14ca7163bdd84bb610e87ea5d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98874
Reviewed-by: Devon Carew <devoncarew@google.com>
Commit-Queue: Kevin Moore <kevmoo@google.com>
kevmoo added a commit to dart-lang/csslib that referenced this issue Apr 10, 2019
* Remove args, logging, path deps – along with binary and lib

Related to dart-lang/sdk#35802

* require a more recent Dart SDK
@kevmoo
Copy link
Member

kevmoo commented Apr 11, 2019

Doing my part. So the next roll of pkg:analyzer will have pkg:html back – which will add pkg:csslib

But! We have dropped logging, utf, and args

dart-lang/csslib#89
dart-lang/html#93
dart-lang/html#94
dart-lang/html#96
dart-lang/html#98
dc17303
7959be5

@matanlurey
Copy link
Contributor

Why is the analyzer checking Android manifests XML files at all?

... much less with an HTML parsing package? I realize we want to offer an awesome Android/Flutter experience, but there is a certain point (perhaps already reached) where we need a functional plugin model and not inlining the knowledge of 50 platforms into the analysis server/SDK.

@kevmoo
Copy link
Member

kevmoo commented Apr 11, 2019

Why is the analyzer checking Android manifests XML files at all?

Just included you on internal thread. tl;dr – long story

@sigurdm
Copy link
Contributor

sigurdm commented Feb 16, 2021

We have to pin every dependency because we're an SDK. If any of our dependencies change in a way that breaks us, it can result in our users being permanently unable to use Flutter, including upgrading. That's existential, so while it's technically a tradeoff, it's not really one we have any choice about.

This argument holds for flutter-tool. But not for the flutter-package that is used by flutter-apps - or am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-package Issues related to package:analyzer area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants