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

Do not add sourcemap markings for indentation #14506

Merged
merged 2 commits into from Apr 29, 2022

Conversation

jridgewell
Copy link
Member

@jridgewell jridgewell commented Apr 29, 2022

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This part 1 of me drastically simplifying the sourcemap generation now that we're off the source-map library.

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 29, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51792/

1 similar comment
@babel-bot
Copy link
Collaborator

babel-bot commented Apr 29, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51792/

if (opts.stdoutContains) {
expect(stdout).toContain(expectStdout);
} else {
fs.writeFileSync(opts.stdoutPath, stdout + "\n");
Copy link
Member Author

@jridgewell jridgewell Apr 29, 2022

Choose a reason for hiding this comment

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

This was very annoying.

} else if (stderr) {
throw new Error("stderr:\n" + stderr);
} catch (e) {
if (!process.env.OVERWRITE) throw e;
Copy link
Member Author

@jridgewell jridgewell Apr 29, 2022

Choose a reason for hiding this comment

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

I'm tired of manually updating fixtures, so I'm adding these in the places that can use it.

"sourcesContent": [
"arr.map(x => x * x);"
],
"mappings": "AAAAA,GAAG,CAACC,GAAJ,CAAQ,UAAAC,CAAC;EAAA,OAAIA,CAAC,GAAGA,CAAR;AAAA,CAAT"
Copy link
Member Author

@jridgewell jridgewell Apr 29, 2022

Choose a reason for hiding this comment

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

The diff here:

- AAAAA,GAAG,CAACC,GAAJ,CAAQ,UAAAC,CAAC;AAAA,SAAIA,CAAC,GAAGA,CAAR;AAAA,CAAT
+ AAAAA,GAAG,CAACC,GAAJ,CAAQ,UAAAC,CAAC;EAAA,OAAIA,CAAC,GAAGA,CAAR;AAAA,CAAT

The changed AAAA,SAAIA -> EAAA,OAAIA is the two spaces used for indentation. A means 0, and E means 2, so we moved the generated column 2 times when inserting the new marking. The S -> O is because markings are deltas, so we subtracted 2 columns from S giving us O.

@@ -273,7 +273,7 @@ class Printer {
this.endsWith(charCodes.lineFeed) &&
str.charCodeAt(0) !== charCodes.lineFeed
) {
this._buf.queue(this._getIndent());
this._buf.queueIndentation(this._getIndent());
Copy link
Member Author

@jridgewell jridgewell Apr 29, 2022

Choose a reason for hiding this comment

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

This is the real change.

/**
* Same as queue, but this indentation will never have a sourcmap marker.
*/
queueIndentation(str: string): void {
Copy link
Member Author

@jridgewell jridgewell Apr 29, 2022

Choose a reason for hiding this comment

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

This is the real change.

const SPACES_RE = /^[ \t]+$/;
export default class Buffer {
constructor(map?: SourceMap | null) {
this._map = map;
}

_map: SourceMap = null;
_buf: string = "";
Copy link
Member Author

@jridgewell jridgewell Apr 29, 2022

Choose a reason for hiding this comment

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

Sorry, but this file needs a lot of cleanup.

@nicolo-ribaudo nicolo-ribaudo added the PR: Polish 💅 A type of pull request used for our changelog categories label Apr 29, 2022
generated: { line: 2, column: 0 },
source: "a.js",
original: { line: 1, column: 20 },
},
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 29, 2022

Choose a reason for hiding this comment

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

Q: why was this deleted?

Copy link
Member Author

@jridgewell jridgewell Apr 29, 2022

Choose a reason for hiding this comment

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

This is { line: 2, column: 0 } of the generated output, which matches with indentation:

function hi(msg) {
  console.log(msg);
}

hi('hello');

If you look at the next object, we have { line: 2, column: 2 }, which matches with the start of console in the generated output.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Apr 29, 2022

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit 518e1a6 into babel:main Apr 29, 2022
34 of 36 checks passed
@jridgewell jridgewell deleted the sourcemap-indentation branch Apr 29, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: sourcemaps outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants