Skip to content

Commit

Permalink
fix: sourcemaps for multiline strings are incorrect
Browse files Browse the repository at this point in the history
The optimization takes advantage of these rules:
1. Written code optionally ends with `lineEnd`
2. Only string and template literals can contain new lines
3. Line comments always end with a new line

Renamed test file.
Moved benchmark tests to its own module.

Closes #247.
  • Loading branch information
connor4312 authored Jan 3, 2021
1 parent 8c3ef44 commit f246aff
Show file tree
Hide file tree
Showing 6 changed files with 150 additions and 104 deletions.
7 changes: 4 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
"build:demo": "npm run build:minified && cp dist/astring.min.* docs/demo/",
"prepare": "npm run build",
"test": "npm run eslint && npm run prettier:check && npm run build:minified && npm run test:coverage",
"test:watch": "ava --watch",
"dev": "ava --watch",
"test:coverage": "c8 --reporter=html --reporter=text --reporter=lcov --include='src/*.js' --exclude='src/tests/**/*.js' ava",
"test:scripts": "npm run test:scripts:build && tap test/_scripts.js",
"test:scripts": "npm run test:scripts:build && ava src/tests/_scripts.js",
"test:benchmark": "ava src/tests/benchmark.js",
"benchmark": "node --require esm ./src/tests/benchmark.js",
"eslint": "eslint src",
"prettier": "prettier --write \"{src,scripts}/**/*.js\" \"bin/astring\"",
Expand Down Expand Up @@ -134,7 +135,7 @@
},
"ava": {
"files": [
"src/**/tests/index.js"
"src/**/tests/astring.js"
],
"require": [
"esm"
Expand Down
100 changes: 66 additions & 34 deletions src/astring.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ function formatComments(state, comments, indent, lineEnd) {
state.write(indent)
if (comment.type[0] === 'L') {
// Line comment
state.write('// ' + comment.value.trim() + '\n')
state.write('// ' + comment.value.trim() + '\n', comment)
} else {
// Block comment
state.write('/*')
Expand Down Expand Up @@ -557,9 +557,9 @@ export const baseGenerator = {
state.write(';')
},
ImportExpression(node, state) {
state.write('import(');
this[node.source.type](node.source, state);
state.write(')');
state.write('import(')
this[node.source.type](node.source, state)
state.write(')')
},
ExportDefaultDeclaration(node, state) {
state.write('export default ')
Expand Down Expand Up @@ -681,9 +681,9 @@ export const baseGenerator = {
state.write('await ', node)
if (node.argument) {
if (node.argument.type === 'ArrowFunctionExpression') {
state.write('(', node)
state.write('(')
this[node.argument.type](node.argument, state)
state.write(')', node)
state.write(')')
} else {
this[node.argument.type](node.argument, state)
}
Expand All @@ -695,16 +695,18 @@ export const baseGenerator = {
const { length } = expressions
for (let i = 0; i < length; i++) {
const expression = expressions[i]
this.TemplateElement(quasis[i], state)
const quasi = quasis[i]
state.write(quasi.value.raw, quasi)
state.write('${')
this[expression.type](expression, state)
state.write('}')
}
state.write(quasis[quasis.length - 1].value.raw)
const quasi = quasis[quasis.length - 1]
state.write(quasi.value.raw, quasi)
state.write('`')
},
TemplateElement(node, state) {
state.write(node.value.raw)
state.write(node.value.raw, node)
},
TaggedTemplateExpression(node, state) {
this[node.tag.type](node.tag, state)
Expand Down Expand Up @@ -824,7 +826,10 @@ export const baseGenerator = {
UnaryExpression(node, state) {
if (node.prefix) {
state.write(node.operator)
if (node.operator.length > 1 || node.argument.type === 'UnaryExpression') {
if (
node.operator.length > 1 ||
node.argument.type === 'UnaryExpression'
) {
state.write(' ')
}
if (
Expand Down Expand Up @@ -962,6 +967,7 @@ export const baseGenerator = {
},
Literal(node, state) {
if (node.raw != null) {
// Non-standard property
state.write(node.raw, node)
} else if (node.regex != null) {
this.RegExpLiteral(node, state)
Expand Down Expand Up @@ -1005,6 +1011,7 @@ class State {
this.lineEndSize = this.lineEnd.split('\n').length - 1
this.mapping = {
original: null,
// Uses the entire state to avoid generating ephemeral objects
generated: this,
name: undefined,
source: setup.sourceMap.file || setup.sourceMap._file,
Expand All @@ -1031,32 +1038,57 @@ class State {
}

map(code, node) {
if (node != null && node.loc != null) {
const { mapping } = this
mapping.original = node.loc.start
mapping.name = node.name
this.sourceMap.addMapping(mapping)
}
if (code.length > 0) {
if (this.lineEndSize > 0) {
if (code.endsWith(this.lineEnd)) {
this.line += this.lineEndSize
this.column = 0
} else if (code[code.length - 1] === '\n') {
// Case of inline comment
this.line++
this.column = 0
} else {
this.column += code.length
if (node != null) {
const { type } = node
if (type[0] === 'L' && type[2] === 'n') {
// LineComment
this.column = 0
this.line++
return
}
if (
(type[0] === 'T' && type[8] === 'E') ||
(type[0] === 'L' && type[1] === 'i' && typeof node.value === 'string')
) {
// TemplateElement or Literal string node
const { length } = code
if (length === 0 || (length === 2 && type[0] === 'L')) {
// Empty TemplateElement or Literal string with begin and end quotes
return
}
} else {
if (code[code.length - 1] === '\n') {
// Case of inline comment
this.line++
this.column = 0
} else {
this.column += code.length
let { column, line } = this
for (let i = 0; i < length; i++) {
if (code[i] === '\n') {
column = 0
line++
} else {
column++
}
}
this.column = column
this.line = line
return
}
if (node.name != null) {
const { mapping } = this
mapping.original = node.loc.start
mapping.name = node.name
this.sourceMap.addMapping(mapping)
}
}
const { length } = code
const { lineEnd } = this
if (length > 0) {
if (
this.lineEndSize > 0 &&
(lineEnd.length === 1
? code[length - 1] === lineEnd
: code.endsWith(lineEnd))
) {
this.line += this.lineEndSize
this.column = 0
} else {
this.column += length
}
}
}
Expand Down
96 changes: 30 additions & 66 deletions src/tests/index.js → src/tests/astring.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import test from 'ava'
import path from 'path'
import { parse } from 'acorn'
import * as astravel from 'astravel'
import { pick } from 'lodash'

import { generate } from '../astring'
import { readFile } from './tools'
import benchmarkWithCode from './benchmark'

const FIXTURES_FOLDER = path.join(__dirname, 'fixtures')

Expand Down Expand Up @@ -123,72 +123,36 @@ test('Comment generation', (assert) => {
})

test('Source map generation', (assert) => {
const code = 'function f(x) {\n return x;\n}\n'
const sourceMap = {
mappings: [],
_file: 'script.js',
addMapping({ original, generated: { line, column }, name, source }) {
const generated = { line, column }
assert.deepEqual(generated, { ...original })
assert.is(source, this._file)
this.mappings.push({
original,
generated,
name,
source,
})
},
}
const ast = parse(code, {
const dirname = path.join(FIXTURES_FOLDER, 'syntax')
const files = fs.readdirSync(dirname).sort()
const options = {
ecmaVersion,
sourceType: 'module',
locations: true,
}
files.forEach((filename) => {
const code = readFile(path.join(dirname, filename))
const sourceMap = {
mappings: [],
_file: 'script.js',
addMapping({ original, generated, name, source }) {
assert.deepEqual(
pick(generated, ['line', 'column']),
pick(original, ['line', 'column']),
`${filename}:${name}`,
)
assert.is(source, this._file)
this.mappings.push({
original,
generated,
name,
source,
})
},
}
const ast = parse(code, options)
generate(ast, {
sourceMap,
})
})
const formattedCode = generate(ast, {
sourceMap,
})
assert.is(sourceMap.mappings.length, 3)
assert.is(formattedCode, code)
})

test.skip('Performance tiny code', (assert) => {
const result = benchmarkWithCode('var a = 2;', 'tiny code')
assert.true(
result['astring'].speed > result['escodegen'].speed,
'astring is faster than escodegen',
)
assert.true(
result['astring'].speed > 10 * result['babel'].speed,
'astring is at least 10x faster than babel',
)
assert.true(
result['astring'].speed > 10 * result['prettier'].speed,
'astring is at least 10x faster than prettier',
)
assert.true(
result['acorn + astring'].speed > result['buble'].speed,
'astring is faster than buble',
)
})

test.skip('Performance with everything', (assert) => {
const result = benchmarkWithCode(
readFile(path.join(FIXTURES_FOLDER, 'tree', 'es6.js')),
'everything',
)
assert.true(
result['astring'].speed > result['escodegen'].speed,
'astring is faster than escodegen',
)
assert.true(
result['astring'].speed > 10 * result['babel'].speed,
'astring is at least 10x faster than babel',
)
assert.true(
result['astring'].speed > 10 * result['prettier'].speed,
'astring is at least 10x faster than prettier',
)
assert.true(
result['acorn + astring'].speed > result['buble'].speed,
'astring is faster than buble',
)
})
45 changes: 45 additions & 0 deletions src/tests/benchmark.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import path from 'path'

import Benchmark from 'benchmark'
import test from 'ava'
import { join, keys, fill, map } from 'lodash'

import { parse as acorn } from 'acorn'
Expand All @@ -16,6 +17,7 @@ import { transform as sucrase } from 'sucrase'

import { readFile } from './tools'

const FIXTURES_FOLDER = path.join(__dirname, 'fixtures')
const SCRIPT = process.argv[1].indexOf('benchmark.js') !== -1

export default function benchmarkWithCode(code) {
Expand Down Expand Up @@ -147,3 +149,46 @@ if (SCRIPT) {
})
console.log(resultsToMarkdown(results))
}

test('Performance tiny code', (assert) => {
const result = benchmarkWithCode('var a = 2;', 'tiny code')
assert.true(
result['astring'].speed > result['escodegen'].speed,
'astring is faster than escodegen',
)
assert.true(
result['astring'].speed > 10 * result['babel'].speed,
'astring is at least 10x faster than babel',
)
assert.true(
result['astring'].speed > 10 * result['prettier'].speed,
'astring is at least 10x faster than prettier',
)
assert.true(
result['acorn + astring'].speed > result['buble'].speed,
'astring is faster than buble',
)
})

test('Performance with everything', (assert) => {
const result = benchmarkWithCode(
readFile(path.join(FIXTURES_FOLDER, 'tree', 'es6.js')),
'everything',
)
assert.true(
result['astring'].speed > result['escodegen'].speed,
'astring is faster than escodegen',
)
assert.true(
result['astring'].speed > 10 * result['babel'].speed,
'astring is at least 10x faster than babel',
)
assert.true(
result['astring'].speed > 10 * result['prettier'].speed,
'astring is at least 10x faster than prettier',
)
assert.true(
result['acorn + astring'].speed > result['buble'].speed,
'astring is faster than buble',
)
})
4 changes: 4 additions & 0 deletions src/tests/fixtures/sourcemap-cases/strings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const a = `hello
world`;
const b = 'hello\
world';
2 changes: 1 addition & 1 deletion src/tests/fixtures/syntax/template.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
let a;
let b = `this is a template`;
let c = `this is a template
let c = `this is a template\
with multiple
lines`;
let d = f`template with function`;
Expand Down

0 comments on commit f246aff

Please sign in to comment.