-
Notifications
You must be signed in to change notification settings - Fork 716
Add support for SourceMap emit and baselines #773
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
Conversation
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.
Copilot reviewed 948 out of 948 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
internal/ast/ast.go:9679
- Consider adding documentation for the new Text() method to clarify its behavior compared to the previous Text field, which will help future maintainers.
func (node *SourceFile) Text() string {
internal/sourcemap/generator.go:23
- [nitpick] Ensure that the renamed 'Generator' type is clearly documented and consistently referenced across the codebase to reduce potential confusion with other generator patterns.
type Generator struct {
internal/sourcemap/util.go
Outdated
) | ||
|
||
var ( | ||
sourceMapCommentRegExp = regexp.MustCompile(`^\/\/[@#] sourceMappingURL=(.+)\r?\n?$`) |
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.
Eventually we'll want to eliminate these since Go's regex perf is pretty bad.
testdata/baselines/reference/submodule/compiler/commonSourceDirectory.js.map.diff
Show resolved
Hide resolved
// !!! Source file from node_modules are not emitted. In Strada, this depends on module resolution and uses | ||
// `sourceFilesFoundSearchingNodeModules` in `createProgram`. For now, we will just check for `/node_modules/` in | ||
// the file name. | ||
if strings.Contains(sourceFile.FileName(), "/node_modules/") { |
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 would suggest using .Pah()
here but I assume this is just temporary?
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, this is temporary.
This wires up source map emit to the emitter and enables source map baselines. After some fixes, the few source map diffs seem to be mostly due to missing transforms.
This also makes a minor change to the
ast.SourceFile
type to makeText()
a method rather than a field. This was partially done to supportsourcemap.Source
, but also to make it easier to access the source file text while debugging as the Go debugger gets confused byText
being a field onSourceFile
but a method onNode
, which is observed when addingfile.Text
as a Watch expression or when referencing it in the Debug console.