-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
fix: js fallback sourcemap content should be using original content #15135
Conversation
/ecosystem-ci run |
@@ -27,6 +27,7 @@ export interface SendOptions { | |||
cacheControl?: string | |||
headers?: OutgoingHttpHeaders | |||
map?: SourceMap | { mappings: '' } | null | |||
originalContent?: string |
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 this be () => string
in case it doesn't end up being used to save the file read?
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.
send
function is a sync function and is part of the public API. Therefore originalContent
cannot be () => Promise<string>
. To satisfy () => string
, we need to change fsp.readFile
to fs.readFileSync
in getOriginalContent
. But I guess async is better for perf.
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 would be good to find a way to avoid the perf regression. In a full .ts code base, there will now be a new read for each transformed file that won't be used, no? I think at one point we could have an internal send
function that is async (everywhere internal usage could be changed to await), but given that it may be complex, for now we could add a guard at https://github.com/vitejs/vite/pull/15135/files#diff-6d94d6934079a4f09596acc9d3f3d38ea426c6f8e98cd766567335d42679ca7cR208, with a comment? Even if it is redundant, we at least save a few reads?
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.
Ah, OK. I noticed I was missing type === '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.
In a full .ts code base, there will now be a new read for each transformed file that won't be used, no?
Just to make it clear, this won't happen. .ts
files will always have source maps, so map == null
is always false
.
Thank you for such a quick response! I also checked in the WebStorm debugger and now everything works fine. |
📝 Ran ecosystem CI on
|
…ontent (vitejs#15135)" This reverts commit 227d56d.
Description
Follow up to #14247.
fixes #14247 (comment)
I tested this change with VSCode, Chrome, Firefox.
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).