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(driver): create eval code using interface #10816
core(driver): create eval code using interface #10816
Changes from 18 commits
90018b2
f7e38f5
78f386a
32856e7
e2dd63d
f38bf41
ce9839b
4c903de
0413a4a
d4bd2bf
02dc215
ce2827a
c81a636
70d8210
df5e8dd
4e59476
147b98c
a4051a1
77afd88
fd94564
9e7644e
f42ca84
b1fb5d3
c4d3a58
ee6f930
6be38eb
97b0525
81ea5b5
4843291
3c9f573
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.
this is the interface exposed to gathering. Are there any docs on
createEvalCode
that could move here?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.
No. But we could copy the contents of createEvalCode here?
I'm surprised
@see
doesn't do anything useful in vscode.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.
ref microsoft/TypeScript#35524
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.
sorry, that's what I meant :) Even
@see
is an extra click (and won't appear on hover)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.
Should we keep this consistent with the style of link-elements?
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.
yeah, which is the way that breaks when minified? If we're getting confused we should definitely document this :)
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, and it is necessary, due to mangling #11463
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 unfortunate, since we lose all
deps
type checking benefits then :/(see also #11463 (comment) for an example of the mangling)
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.
yea, reminder tho that we could get it back if we namespace the page functions (idea 3 #10781 )
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.
could this be directly incorporated into
evaluate()
(andevaluateFunctionOnObject
) instead of splitting it into this file? It's mostly not shared code since the two callers take different branches in the conditional (and it's not really a "page function" in the same way as the other functions in this file are).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 prefer it here bc/ it's easier to test (driver has some overhead with
beforeEach
etc.) andkeeps the driver file smaller (it's so big). Maybe
lib/eval.js
?And then export two function:
createFunctionEvalCode
andcreateIffeEvalCode
?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.
well it would definitely be good to split the two, but it'll also have a new home after #11633, and it also looks trivial to add to
.evaluateAsync
indriver-test
?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.
@patrickhulce would you prefer this going into
driver.js
or a new filelib/eval.js
?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.
new
lib/eval.js
file SGTM, though I'll also +1 @brendankenny's test suggest to make sure.evaluate
uses it :)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.
Isn't the (currently named)
RuntimeController
going to be this already in #11633, though? I guess I don't understand what the problem is just inlining the five lines or so :)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.
Ya, that's why I don't really care much where it lives in this PR :)
I mean I don't feel terribly strongly about it, the rebase gets slightly more annoying with inline but not by much.
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:
join()
on an empty list is an empty string anyway. You can get rid of the conditionals.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.
args and deps are optional params.
strangely, typecheck found no error when I removed the conditionals, even though both were marked as
Array | undefined
...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.
it's because this file is
// @ts-nocheck
. Maybe after this change and followup(s), we could drop that with a minimum of expect-errors