-
Notifications
You must be signed in to change notification settings - Fork 363
Lookup the package config location for Dart and Flutter test targets #7941
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
Changes from 5 commits
b194db9
df5f111
8e2621f
5fe69dd
8a3151f
e67d5e0
f2418a8
2014a37
a3530bb
0105a29
75eb135
92af18b
7b2b3aa
3694576
4a713f6
a819156
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,5 +2,20 @@ | |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'package:path/path.dart' as path; | ||
|
|
||
| /// Describes an instance of the Dart Tooling Daemon. | ||
| typedef DTDConnectionInfo = ({String? uri, String? secret}); | ||
|
|
||
| /// The name of the Directory where a Dart application's package config file is | ||
| /// stored. | ||
| const dartToolDirectoryName = '.dart_tool'; | ||
|
|
||
| /// The name of the package config file for a Dart application. | ||
| const packageConfigFileName = 'package_config.json'; | ||
|
|
||
| /// The path identifier for the package config URI for a Dart application. | ||
| /// | ||
| /// The package config file lives at '.dart_tool/package_config.json'. | ||
| final packageConfigIdentifier = | ||
|
||
| path.join(dartToolDirectoryName, packageConfigFileName); | ||
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.
@jakemac53 @natebosch @bkonyi is this backwards compatibility required? DevTools is pinned to the Dart SDK version. Is
package:testalso? Or is it possible for a user to be on a new Dart SDK with these DevTools changes, but an old version ofpackage:testthat does not have the generated constant we are trying to eval?Uh oh!
There was an error while loading. Please reload this page.
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.
Yes, it's possible for a new SDK to be used with an old
package:test. We should not rely on these always being present. We also should be resilient if the strings have the content'null'in case these code paths are used in places whereIsolate.packageConfigreturnsnull.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.
Sounds good. The current code covers that case, so we should be good to go. In the case you described where
Isolate.packageConfigreturns 'null`, would we still want to fallback to the case where we are checking the test import prefix? From the DevTools perspective, I don't mind more fallbacks that if it allows us to provide a more robustness experience to the user.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 don't know of any concrete reasons for getting a
null, so it's hard to say whether the normal fallback behavior would help or not. In any case I don't think it hurts to try a fallback approach for this situation. I mentioned it because the API is nullable and we are explicitly not doing anything special with it there. If it would be useful to omit the variable entirely over writing the string 'null' we could do that too.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.
Let's leave the variable even if it writes null. If the variable is missing, the evaluation would throw, so I think it is better to just rely on our string comparison to cover this case.