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
chore: implement experimental ESM stub/spy for Vite #26536
chore: implement experimental ESM stub/spy for Vite #26536
Changes from all commits
f36b5ae
99f9c77
7a981c2
585bc21
f379848
133fecc
691a67d
5fc6ed9
3631bc9
2542c07
149295a
b4960a5
0ff31e2
0ed8348
efee5e2
0b2fe72
2e6c4c7
28f6733
d1e81f0
ecb3139
9ca77b2
ef1affe
ec16a68
cc37d37
fbd11ff
555605b
36c1cb0
45e7f5f
5a3615b
ad5e893
45c2002
befd9dc
d3819df
4cad299
74ac1b7
b8fe483
4179a2d
d123e37
f33dc9a
6a4665c
7f227c6
a56e2a0
416c542
c223209
6551833
245330b
f625ce6
7e13042
15642c3
0df9ddd
d3cf76b
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.
nitpick: This duplicates the function wrapping we do on line 45, could pull out a
wrapFunction
helper just to make sure they stay alignedThere 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.
Now that I think about it I can't remember why I found this block necessary. Theoretically all functions should have been proxied up front 🤔
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 tried to make this refactor, it's quite awkward and I think it's a little less clear to read. Any recommendation for how you think it could be better written?
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.
Just wondering now that we have more custom logic in this area if we'll need to apply it (eg constructor handling) in other places we're building function wrappers - there's like 4 places we're generating wrappers, hard to keep track of which ones need to account for what.
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.
We can probably rejigger this a bit now - we've got decent enough test coverage.
Would also be good to get this merged prior to the sprint ending - I could go either way. Hard to say how much more we will work on this in the near future, given the caveats we've run into (like the reference vs proxy comparison that might be unsolvable).