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

fix line numbers when inline sourcemap is used #3717

Merged
merged 3 commits into from
Apr 26, 2024
Merged

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Apr 26, 2024

What?

Fix sourcemaps when they are inlined.

Before that if there was an inline sourcemap we won't load it and go through the already used method of updating it.

This isn't a problem if babel actually transpiles it as it will load it, but in case the sourcecode was already transpiled with inline maps this will now update the lines correctly.

Why?

To have correct lines when people user sourcemaps .

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#3689

@mstoykov mstoykov added the bug label Apr 26, 2024
@mstoykov mstoykov added this to the v0.51.0 milestone Apr 26, 2024
@mstoykov mstoykov requested a review from a team as a code owner April 26, 2024 13:10
@mstoykov mstoykov requested review from codebien and olegbespalov and removed request for a team April 26, 2024 13:10
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 73.28%. Comparing base (1b0088c) to head (d3c4f51).
Report is 3 commits behind head on master.

❗ Current head d3c4f51 differs from pull request most recent head 4a3ecee. Consider uploading reports for the commit 4a3ecee to get more accurate results

Files Patch % Lines
js/compiler/compiler.go 65.21% 4 Missing and 4 partials ⚠️
js/modules/resolution.go 33.33% 1 Missing and 1 partial ⚠️
js/modulestest/runtime.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3717      +/-   ##
==========================================
- Coverage   73.31%   73.28%   -0.03%     
==========================================
  Files         278      278              
  Lines       20425    20456      +31     
==========================================
+ Hits        14975    14992      +17     
- Misses       4499     4507       +8     
- Partials      951      957       +6     
Flag Coverage Δ
ubuntu 73.20% <66.66%> (-0.03%) ⬇️
windows 73.14% <66.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mstoykov mstoykov requested a review from szkiba April 26, 2024 13:21
// the lines in the sourcemap (if available) will be fixed by increaseMappingsByOne
conditionalNewLine = "\n"
newCode, err := state.updateInlineSourceMap(code, index)
if err != nil {
c.logger.Warn("while compiling %q we couldn't update it's inline sourcemap which might lead "+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me as a user, if I can fix it in some way. Do we have a suggestion? Or the answer is there isn't much you can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue this warning is mostly so that when they report an issue - they report it with this warning

Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of the originally attached script, the line number has been corrected. At the same time, if I transfer the throw expression to another function, it no longer works.

interface User {
  name: string;
  id: number;
}

class UserAccount implements User {
  name: string;
  id: number;

  constructor(name: string) {
    this.name = name;
    this.id = Math.floor(Math.random() * Number.MAX_SAFE_INTEGER);
  }
}

function newUser(name: string): User {
  throw "other";
  return new UserAccount(name);
}

export { User, newUser };

@mstoykov
Copy link
Contributor Author

@szkiba - I used the wrong base64 encoding, that doesn't support padding , and just so happened the original example works with it 😓 , but not this new one 🤦

I have now fixed it as well as fixing the warning message a bit

szkiba
szkiba previously approved these changes Apr 26, 2024
Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works now.

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a small text suggestion

js/compiler/compiler.go Outdated Show resolved Hide resolved
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mstoykov mstoykov merged commit f38ac59 into master Apr 26, 2024
22 checks passed
@mstoykov mstoykov deleted the fixInlineSourceMap branch April 26, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants