Skip to content

Added method findPackageConfigFilePath to find the package_config.json file #2587

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 11, 2025
Merged

Added method findPackageConfigFilePath to find the package_config.json file #2587

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ccb5514
added findPackageConfigFilePath to find the package_config.json file
jyameo Feb 4, 2025
720b74b
updated Changelog
jyameo Feb 4, 2025
adc54a6
updated strategy providers to allow clients to specify package config…
jyameo Feb 5, 2025
2d7800d
added util method for webdev to compute package config path and pass …
jyameo Feb 5, 2025
3d837f5
updated docstring
jyameo Feb 5, 2025
4361530
revert changes to webdev
jyameo Feb 7, 2025
0bb1a44
Set DWDS version to 24.3.5 to prepare for release
jyameo Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dwds/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## 24.3.4

- Ensure `packageConfigPath` is initialized in `LoadStrategy` at construction.
- Added support for some debugging APIs with the DDC library bundle format. - [#2566](https://github.com/dart-lang/webdev/issues/2566), [#2573](https://github.com/dart-lang/webdev/issues/2573)
- Added support for hot reload using the DDC library bundle format.

Expand Down
27 changes: 26 additions & 1 deletion dwds/lib/src/loaders/strategy.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:io';
import 'dart:typed_data';

import 'package:dwds/src/debugging/dart_runtime_debugger.dart';
Expand All @@ -20,7 +21,7 @@ abstract class LoadStrategy {
LoadStrategy(
this._assetReader, {
String? packageConfigPath,
}) : _packageConfigPath = packageConfigPath;
}) : _packageConfigPath = packageConfigPath ?? _findPackageConfigFilePath();
Copy link
Contributor

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 FrontendServerStrategyProviders e.g.:

webdev/dwds/lib/src/loaders/frontend_server_strategy_provider.dart

Line 156 in 616da45

FrontendServerDdcLibraryBundleStrategyProvider(
and avoid computing this in DWDS.

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).

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.


/// The ID for this strategy.
///
Expand Down Expand Up @@ -83,6 +84,30 @@ abstract class LoadStrategy {
'package_config.json',
);

/// Returns the absolute file path of the `package_config.json` file in the `.dart_tool`
/// directory, searching recursively from the current directory hierarchy.
/// If `_packageConfigPath` is already set, it returns the cached value immediately.
static String? _findPackageConfigFilePath() {
var candidateDir = Directory(DartUri.currentDirectory).absolute;

while (true) {
final candidatePackageConfigFile =
File(p.join(candidateDir.path, '.dart_tool', 'package_config.json'));

if (candidatePackageConfigFile.existsSync()) {
return candidatePackageConfigFile.path;
}

final parentDir = candidateDir.parent;
if (parentDir.path == candidateDir.path) {
// We've reached the root directory
return null;
}

candidateDir = parentDir;
}
}

/// Returns the bootstrap required for this [LoadStrategy].
///
/// The bootstrap is appended to the end of the entry point module.
Expand Down
Loading