Skip to content
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

Refactor JS sourcemap generator #21053

Merged
merged 15 commits into from
Dec 13, 2022
Merged

Conversation

ire4ever1190
Copy link
Contributor

@ire4ever1190 ire4ever1190 commented Dec 9, 2022

Refactors the source map generator so that line information is properly serialised into source map

Closes #21052

The refactoring allows for better source maps since we now have column info stored also.
For example stack traces now map to nim lines

for i in 0..<100:
  let x = i + 1
  if x > 0:
    echo i

let y = 9 + 100

proc main() = 
  raise (ref CatchableError)(msg: "test")

main()

image

Didn't have space after the path
Sources would sometimes double up with a new line at the end
@ire4ever1190 ire4ever1190 marked this pull request as draft December 9, 2022 02:26
@ire4ever1190 ire4ever1190 marked this pull request as draft December 9, 2022 02:26
Removes the multiple translations needed, now goes from single high level type to the final SourceMap

Adds documentation for procs
@ire4ever1190 ire4ever1190 changed the title Fix JS source maps not having line information Refactor JS sourcemap generator Dec 9, 2022
Files aren't linking correctly though
Lines are sometimes off but overall seems pretty good

Just need to implement parser
@Yu-Vitaqua-fer-Chronos
Copy link

Does any work need to be done on this still?

@ire4ever1190
Copy link
Contributor Author

Just need to reimplement the tokenizer. Other than that it works fairly well.

@ire4ever1190 ire4ever1190 marked this pull request as ready for review December 10, 2022 02:05
@ire4ever1190
Copy link
Contributor Author

Just finishing up writing a test case and then it should be all ready

@Araq
Copy link
Member

Araq commented Dec 12, 2022

Hurry up please, we need to have this in 2.0 RC 1.

@ire4ever1190
Copy link
Contributor Author

Ok test case is written and passes locally. Should stop this regression in future

@Araq
Copy link
Member

Araq commented Dec 12, 2022

The CI isn't happy though:

FAIL: tests/js/tsourcemap.nim js -d:release

You can test this locally via:


nim c -r testament/testament run tests/js/tsourcemap.nim

@ire4ever1190
Copy link
Contributor Author

Running that still passes locally so think I might have a config difference somewhere.
I now read files from outdir instead of relative to the file so think that might've been the problem

@Araq Araq added the merge_when_passes_CI mergeable once green label Dec 12, 2022
@ringabout ringabout merged commit 1fefb8e into nim-lang:devel Dec 13, 2022
@ire4ever1190 ire4ever1190 deleted the fix-21052 branch December 13, 2022 04:22
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 1fefb8e

Hint: mm: orc; opt: speed; options: -d:release
165478 lines; 8.183s; 612.242MiB peakmem

survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
* Parse the new line format

* Fix pattern

Didn't have space after the path

* Remove duplicate sources

Sources would sometimes double up with a new line at the end

* Remove unused variable

* Refactor sourcemap.nim

Removes the multiple translations needed, now goes from single high level type to the final SourceMap

Adds documentation for procs

* Line numbers line up properly now

Files aren't linking correctly though

* Files now link up correctly

Lines are sometimes off but overall seems pretty good

Just need to implement parser

* Add column info to output

Add sourceMappingURL to rope directly to prevent copy

* Properly handle columns

* Remove debug lines

* Add testcase

* Finish testcase

* Use the outdir folder instead of the folder the test is in to find the sourcemap

Co-authored-by: ringabout <43030857+ringabout@users.noreply.github.com>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* Parse the new line format

* Fix pattern

Didn't have space after the path

* Remove duplicate sources

Sources would sometimes double up with a new line at the end

* Remove unused variable

* Refactor sourcemap.nim

Removes the multiple translations needed, now goes from single high level type to the final SourceMap

Adds documentation for procs

* Line numbers line up properly now

Files aren't linking correctly though

* Files now link up correctly

Lines are sometimes off but overall seems pretty good

Just need to implement parser

* Add column info to output

Add sourceMappingURL to rope directly to prevent copy

* Properly handle columns

* Remove debug lines

* Add testcase

* Finish testcase

* Use the outdir folder instead of the folder the test is in to find the sourcemap

Co-authored-by: ringabout <43030857+ringabout@users.noreply.github.com>
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
* Parse the new line format

* Fix pattern

Didn't have space after the path

* Remove duplicate sources

Sources would sometimes double up with a new line at the end

* Remove unused variable

* Refactor sourcemap.nim

Removes the multiple translations needed, now goes from single high level type to the final SourceMap

Adds documentation for procs

* Line numbers line up properly now

Files aren't linking correctly though

* Files now link up correctly

Lines are sometimes off but overall seems pretty good

Just need to implement parser

* Add column info to output

Add sourceMappingURL to rope directly to prevent copy

* Properly handle columns

* Remove debug lines

* Add testcase

* Finish testcase

* Use the outdir folder instead of the folder the test is in to find the sourcemap

Co-authored-by: ringabout <43030857+ringabout@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sourcemaps don't include line number information
4 participants