Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[rush] Subspace Configuration Files #4442
[rush] Subspace Configuration Files #4442
Changes from 5 commits
0239c70
b004a66
1bd9b71
d919129
3b755ee
2fbdc7d
2d53710
8f91e8a
d0370eb
c27c5ae
47e3e89
8c8b539
b7e1afd
8df0788
306ee40
d2ed1f2
5b1b623
1705bc6
bdf0c9e
cf056e8
43b274f
b5d58c6
8f65295
8d42b59
d81881c
bf31ea6
0aa57a4
c8e85bf
fa0c483
64f1628
db32b88
67c0c5f
bf2cd20
7184e21
f7f4cfd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can you rename
loadFromConfigurationFile
totryLoadFromConfigurationFile
and just return undefined if the file doesn't exist?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.
And ideally this load should be async.
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.
Changed to tryLoadFromConfigurationFile
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.
Can this be made async?
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.
(If the other similar Rush functions are not already
async
, then we shouldn't insist that Will fixes it 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'm a bit confused as to why this would be async - it only uses a call to
FileSystem.exists
, which as far as I can tell is a synchronous action, so if there are no asynchronous calls within thetryLoadFromConfigurationFile
function, what purpose would labelling it async be?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.
There is a
FileSystem.existsAsync
, and there is aJsonFile.loadAndValidateAsync
. Anything that touches the filesystem (including JSON file loading) should ideally be async to allow those calls to be parallelized.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.
Ideally all of the things that touch the filesystem should be async.
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.
This function only calls FileSystem.exists, which is a synchronous function - does it still need to be labelled async?
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.
@iclanton
RushConfiguration.tryLoadFromDefaultLocation()
itself is synchronous. Synchronous functions cannot callasync
functions. Therefore I think we may be asking @william2958 to do nontrivial work that is out of scope for 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.
Discarding every possible error is always a suspicious practice.
Looking up the implementation in true-case-path/index.js, there are actually 3 separate errors that it can throw (aside from indirect errors such as
Cannot read properties of undefined
):[true-case-path]: Called with ${filePath}, but no matching file exists
[true-case-path]: basePath argument must be absolute. Received "${basePath}"
[true-case-path]: filePath must be relative when used with basePath
The first one is probably what we're hoping to catch, whereas inadvertently catching the others would probably hide a bug.
I found
/* ignore errors from true-case-path */
in two other places in the Rush code base, so probably @william2958 is simply copying that practice. Thus addressing this is out of scope for this PR.But in the future maybe someone should move
true-case-path
into a library function and invent a more correct solution for nonexistent files.@iclanton @dmichon-msft