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
core(lantern): add interface for network request #15845
core(lantern): add interface for network request #15845
Changes from 7 commits
0786eb5
25471ba
f37fc61
36a9ff9
5e54849
d2a438e
cb781ac
02a0504
7bc9679
291dc23
b0450ab
06b2125
d4ee2ef
8c1c375
c0cfa56
408109e
eb193b0
03df941
6dfd8f0
6432824
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.
Assuming
CPUNode
does eventually need a generic, this type asserts thatT
will be the same betweenNetworkNode
andCPUNode
which would never be the case. Maybe we should just remove the generic fromCPUNode
for now?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 is a requirement for type narrowing to continue to work.
Remove the type parameter from BaseNode, CPUNode, or Node, and in the above
node.record
is any.Check failure on line 2 in core/lib/dependency-graph/lantern.d.ts
GitHub Actions / basics
Check failure on line 2 in core/lib/dependency-graph/lantern.d.ts
GitHub Actions / basics
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 it's kinda confusing to have
record
andrequest
properties. Could we renamerecord
tointernalRecord
/recordImpl
or something to emphasize which one is the normal use case and which one is for debugging.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 kept record in this PR just to prevent a big name churn in our usage of Lantern. We can rename them, in another PR to keep this one clearer. I don't know what good names would be right now.
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.
nit: I wouldn't expect this type cast to be necessary, are there errors if it's removed?
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 removes undefined from the type from
.record
, which is that way to make our NetworkRequest assignable to Lantern.NetworkRequest.redirectDestination: NetworkRequest<T>
is the issue. Resolving this won't be trivial, and should be done in a separate PR if at all.Check warning on line 53 in core/lib/dependency-graph/network-node.js
Codecov / codecov/patch
core/lib/dependency-graph/network-node.js#L53