-
Notifications
You must be signed in to change notification settings - Fork 80
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
Added method findPackageConfigFilePath to find the package_config.json file #2587
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on Jessy!
@@ -20,7 +21,7 @@ abstract class LoadStrategy { | |||
LoadStrategy( | |||
this._assetReader, { | |||
String? packageConfigPath, | |||
}) : _packageConfigPath = packageConfigPath; | |||
}) : _packageConfigPath = packageConfigPath ?? _findPackageConfigFilePath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should instead provide this as an optional named parameter (optional named to avoid a breaking change) to each of the FrontendServerStrategyProvider
s e.g.:
FrontendServerDdcLibraryBundleStrategyProvider( |
Flutter already calculates this, so we'd end up calculating it again. Flutter may also choose to configure the locations differently in tests for example.
I'd then imagine webdev calculating this separately and passing it to the provider (I guess in its case it'd use the BuildRunnerRequireStrategyProvider
, which should also be updated in this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this approach and it makes sense to update each of the providers to allow passing a _packageConfigPath
. It works for the flutter tools
case since the info is already computed, we can simply pass it to the provider. However, for Webdev
we don't have this details so what about we allow setting the value of _packageConfigPath
for all the providers but in case we don't pass this value explicitly, we can compute it with _findPackageConfigFilePath
. That way we can ensure that we always have this value set in all cases. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original thought was maybe webdev should calculate it separately and then pass it to DWDS, but maybe it doesn't matter. I do like the idea of our default being a bit more robust for the case of monorepos, so I think your plan makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a similar method in Webdev to find the package config path and pass to DWDS and I also kept the default method in the load strategy class in case the value is not being passed.
…it to the load strategy
@srujzs I'm not too familiar with the release process but It looks like I need to update |
I'd avoid making changes to webdev in this PR as it relies on DWDS and I remember the bots having trouble publishing both versions at once. I'd do the following if you want to publish webdev with a new version of dwds:
|
Added a method that returns the absolute file path of the
package_config.json
file in the.dart_tool
directory, by searching recursively from the current directory hierarchy to ensurepackageConfigPath
is initialized inLoadStrategy
at construction.Fixes #2575