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.

Sign up for GitHub

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

Jump to bottom

Do not add sourcemap markings for indentation #14506

Merged
merged 2 commits into from Apr 29, 2022
Merged

Do not add sourcemap markings for indentation #14506

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbdaebf
Do not add sourcmap markings for identation
jridgewell Apr 29, 2022
6dbc07d
Update tests
jridgewell Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
111 changes: 65 additions & 46 deletions packages/babel-generator/src/buffer.ts
View file
Open in desktop
Expand Up @@ -2,37 +2,51 @@ import type SourceMap from "./source-map";
import type * as t from "@babel/types";
import * as charcodes from "charcodes";

type Pos = {
line: number;
column: number;
};
type Loc = {
start?: Pos;
end?: Pos;
identifierName?: string;
filename?: string;
};
type SourcePos = {
identifierName: string | undefined;
line: number | undefined;
column: number | undefined;
filename: string | undefined;
force: boolean;
};

function SourcePos(): SourcePos {
return {
identifierName: undefined,
line: undefined,
column: undefined,
filename: undefined,
force: false,
};
}

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

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

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.

_last: number = 0;
_queue: Array<
[
str: string,
line: number,
column: number,
identifierName: string | null,
filename: string | null | undefined,
force: boolean | undefined,
]
> = [];

_position: any = {
_buf = "";
_last = 0;
_queue: Parameters<Buffer["_append"]>[] = [];

_position = {
line: 1,
column: 0,
};
_sourcePosition: any = {
identifierName: null,
line: null,
column: null,
filename: null,
};
_disallowedPop: any | null = null;
_sourcePosition = SourcePos();
_disallowedPop: SourcePos | null = null;

/**
* Get the final string output from the buffer, along with the sourcemap if one exists.
Expand Down Expand Up @@ -82,7 +96,6 @@ export default class Buffer {
/**
* Add a string to the buffer than can be reverted.
*/

queue(str: string): void {
// Drop trailing spaces when a newline is inserted.
if (str === "\n") {
Expand All @@ -96,15 +109,22 @@ export default class Buffer {
this._queue.unshift([str, line, column, identifierName, filename, force]);
}

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

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.

this._queue.unshift([
str,
undefined,
undefined,
undefined,
undefined,
false,
]);
}

_flush(): void {
let item: [
string,
number,
number,
string | null | undefined,
string | null | undefined,
boolean | undefined,
];
let item: Parameters<Buffer["_append"]>;
while ((item = this._queue.pop())) {
this._append(...item);
}
Expand All @@ -114,9 +134,9 @@ export default class Buffer {
str: string,
line: number,
column: number,
identifierName?: string | null,
filename?: string | null,
force?: boolean,
identifierName: string | undefined,
filename: string | undefined,
force: boolean,
): void {
this._buf += str;
this._last = str.charCodeAt(str.length - 1);
Expand Down Expand Up @@ -323,23 +343,22 @@ export default class Buffer {
_disallowPop(prop: string, loc: t.SourceLocation) {
if (prop && !loc) return;

this._disallowedPop = this._normalizePosition(prop, loc);
this._disallowedPop = this._normalizePosition(
prop,
loc,
SourcePos(),
false,
);
}

_normalizePosition(prop: string, loc: any, targetObj?: any, force?: boolean) {
_normalizePosition(
prop: string,
loc: Loc | undefined | null,
targetObj: SourcePos,
force: boolean,
) {
const pos = loc ? loc[prop] : null;

if (targetObj === undefined) {
// Initialize with fields so that the object doesn't change shape.
targetObj = {
identifierName: null,
line: null,
column: null,
filename: null,
force: false,
};
}

const origLine = targetObj.line;
const origColumn = targetObj.column;
const origFilename = targetObj.filename;
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-generator/src/printer.ts
View file
Open in desktop
Expand Up @@ -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

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.

}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/babel-generator/src/source-map.ts
View file
Open in desktop
Expand Up @@ -82,7 +82,7 @@ export default class SourceMap {
const generatedLine = generated.line;

// Adding an empty mapping at the start of a generated line just clutters the map.
if (this._lastGenLine !== generatedLine && line === null) return;
if (this._lastGenLine !== generatedLine && line == null) return;

// If this mapping points to the same source location as the last one, we can ignore it since
// the previous one covers it.
Expand Down
16 changes: 11 additions & 5 deletions packages/babel-generator/test/fixtures/sourcemaps/call-identifiers/source-map.json
View file
Open in desktop
@@ -1,9 +1,15 @@
{
"mappings": "AAAAA,GAAG;AACHA,GAAG;AACHA,GAAG,GAAGC,GAAN;AACAC,GAAG,CAACF,GAAJ;AACAE,GAAG,CAACF,GAAJ;AACAE,GAAG,CAACF,GAAJ,CAAQC,GAAR;AACAC,GAAG,CAACF,GAAJ,GAAUC,GAAV;AACA;AACED,EAAAA,GAAG;AACHA,EAAAA,GAAG;AACHA,EAAAA,GAAG,GAAGC,GAAN;AACAC,EAAAA,GAAG,CAACF,GAAJ;AACAE,EAAAA,GAAG,CAACF,GAAJ;AACAE,EAAAA,GAAG,CAACF,GAAJ,CAAQC,GAAR;AACAC,EAAAA,GAAG,CAACF,GAAJ,GAAUC,GAAV;AACD",
"names": ["foo", "bar", "obj"],
"sources": ["fixtures/sourcemaps/call-identifiers/input.js"],
"version": 3,
"names": [
"foo",
"bar",
"obj"
],
"sources": [
"fixtures/sourcemaps/call-identifiers/input.js"
],
"sourcesContent": [
"foo;\nfoo();\nfoo().bar;\nobj.foo;\nobj.foo();\nobj.foo.bar;\nobj.foo().bar;\n{\n foo;\n foo();\n foo().bar;\n obj.foo;\n obj.foo();\n obj.foo.bar;\n obj.foo().bar;\n}"
],
"version": 3
}
"mappings": "AAAAA,GAAG;AACHA,GAAG;AACHA,GAAG,GAAGC,GAAN;AACAC,GAAG,CAACF,GAAJ;AACAE,GAAG,CAACF,GAAJ;AACAE,GAAG,CAACF,GAAJ,CAAQC,GAAR;AACAC,GAAG,CAACF,GAAJ,GAAUC,GAAV;AACA;EACED,GAAG;EACHA,GAAG;EACHA,GAAG,GAAGC,GAAN;EACAC,GAAG,CAACF,GAAJ;EACAE,GAAG,CAACF,GAAJ;EACAE,GAAG,CAACF,GAAJ,CAAQC,GAAR;EACAC,GAAG,CAACF,GAAJ,GAAUC,GAAV;AACD"
}
28 changes: 12 additions & 16 deletions packages/babel-generator/test/index.js
View file
Open in desktop
Expand Up @@ -54,8 +54,7 @@ describe("generation", function () {
version: 3,
sources: ["a.js", "b.js"],
mappings:
// eslint-disable-next-line max-len
"AAAA,SAASA,EAAT,CAAaC,GAAb,EAAkB;AAAEC,EAAAA,OAAO,CAACC,GAAR,CAAYF,GAAZ;AAAmB;;ACAvCD,EAAE,CAAC,OAAD,CAAF",
"AAAA,SAASA,EAAT,CAAaC,GAAb,EAAkB;EAAEC,OAAO,CAACC,GAAR,CAAYF,GAAZ;AAAmB;;ACAvCD,EAAE,CAAC,OAAD,CAAF",
names: ["hi", "msg", "console", "log"],
sourcesContent: [
"function hi (msg) { console.log(msg); }\n",
Expand Down Expand Up @@ -103,12 +102,6 @@ describe("generation", function () {
source: "a.js",
original: { line: 1, column: 18 },
},
{
name: "console",
generated: { line: 2, column: 0 },
source: "a.js",
original: { line: 1, column: 20 },
},
Copy link
Member

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

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.

{
name: "console",
generated: { line: 2, column: 2 },
Expand Down Expand Up @@ -219,7 +212,7 @@ describe("generation", function () {
version: 3,
sources: ["inline"],
names: ["foo", "bar"],
mappings: "AAAA,SAASA,IAAT,GAAe;AAAEC,EAAAA,IAAG;AAAG",
mappings: "AAAA,SAASA,IAAT,GAAe;EAAEC,IAAG;AAAG",
sourcesContent: ["function foo() { bar; }\n"],
},
"sourcemap was incorrectly generated",
Expand Down Expand Up @@ -251,12 +244,6 @@ describe("generation", function () {
source: "inline",
original: { line: 1, column: 15 },
},
{
name: "bar",
generated: { line: 2, column: 0 },
source: "inline",
original: { line: 1, column: 17 },
},
{
name: "bar",
generated: { line: 2, column: 2 },
Expand Down Expand Up @@ -867,7 +854,16 @@ suites.forEach(function (testSuite) {
const result = run();

if (options.sourceMaps) {
expect(result.map).toEqual(task.sourceMap);
try {
expect(result.map).toEqual(task.sourceMap);
} catch (e) {
if (!process.env.OVERWRITE || !task.sourceMapFile) throw e;
console.log(`Updated test file: ${task.sourceMapFile.loc}`);
fs.writeFileSync(
task.sourceMapFile.loc,
JSON.stringify(result.map, null, 2),
);
}
}

if (
Expand Down
7 changes: 7 additions & 0 deletions packages/babel-helper-fixtures/src/index.ts
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ type Test = {
// todo(flow->ts): improve types here
sourceMappings;
sourceMap;
sourceMapFile: TestFile;
};

type Suite = {
Expand Down Expand Up @@ -165,6 +166,7 @@ function pushTask(taskName, taskDir, suite, suiteName) {
},
sourceMappings: undefined,
sourceMap: undefined,
sourceMapFile: undefined,
inputSourceMap: undefined,
};

Expand Down Expand Up @@ -225,6 +227,11 @@ function pushTask(taskName, taskDir, suite, suiteName) {
const sourceMapLoc = taskDir + "/source-map.json";
if (fs.existsSync(sourceMapLoc)) {
test.sourceMap = JSON.parse(readFile(sourceMapLoc));
test.sourceMapFile = {
loc: sourceMapLoc,
code: test.sourceMap,
filename: "",
};
}

const inputMapLoc = taskDir + "/input-source-map.json";
Expand Down