-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
JS Source Maps support #506
Changes from 50 commits
fedbd52
3f31fc5
b09d15f
80b3431
392bbc9
e616cc2
1d90425
51bfe22
2d393d2
85c7587
3c3cb77
9e7e46b
44f7a89
f24bdd7
a5b5ce2
243d543
a2056df
c3f40bd
f6cadb5
a6a0d9a
cef8a8b
5157b02
ca829e5
6d8f4da
d101c48
7d9532b
35a1b6f
156736f
40fe5bc
4840a59
42c52b6
607d5a0
6e89578
bb74a14
897b494
401b12c
b28c167
ec0f552
220bdf8
d0498f9
603a12d
0c97ebf
7e29f10
be46eee
e09aaa2
032d475
b7f6500
05741c0
126f522
2bc0f2b
dd5a018
46f2054
f942bfe
42598ff
8b9ad40
cf4f426
11a5010
d3c86bd
9568714
1222fb9
c013456
06a48b9
56e6ea6
5567ee5
c4eb40f
310d22f
be42812
0e57597
2abb918
f39c21e
90b85d1
8c81116
0b95aae
9908e9e
01db40c
11b89c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,4 +11,4 @@ lib | |
!test/**/node_modules | ||
.vscode/ | ||
.idea/ | ||
*.min.js | ||
*.min.js |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ const fsVisitor = require('../visitors/fs'); | |
const babel = require('../transforms/babel'); | ||
const generate = require('babel-generator').default; | ||
const uglify = require('../transforms/uglify'); | ||
const sourceMaps = require('../utils/SourceMaps'); | ||
|
||
const IMPORT_RE = /\b(?:import\b|export\b|require\s*\()/; | ||
const GLOBAL_RE = /\b(?:process|__dirname|__filename|global|Buffer)\b/; | ||
|
@@ -117,16 +118,41 @@ class JSAsset extends Asset { | |
} | ||
|
||
generate() { | ||
// TODO: source maps | ||
let code = this.isAstDirty | ||
? generate(this.ast).code | ||
: this.outputCode || this.contents; | ||
let code; | ||
if (this.isAstDirty) { | ||
let opts = { | ||
sourceMaps: this.options.sourcemaps, | ||
sourceFileName: this.options.sourcemaps ? this.relativename : undefined | ||
}; | ||
let generated = generate(this.ast, opts, this.contents); | ||
if (this.sourcemap && generated.map) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When/why does this case occur (there is already a source map)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is when babel has to generate the code from ast after for example Typescript parsed the code. Typescript sets a sourcemap, although babel changes the sourcemap in these particular cases, and this function offsets it correctly again. |
||
this.sourcemap = sourceMaps.extendSourceMap( | ||
this.sourcemap, | ||
generated.map | ||
); | ||
} else { | ||
this.sourcemap = this.options.sourcemaps ? generated.map : undefined; | ||
} | ||
code = generated.code; | ||
} | ||
code = code ? code : this.outputCode || this.contents; | ||
|
||
if (this.options.sourcemaps) { | ||
this.sourcemap = this.sourcemap | ||
? this.sourcemap | ||
: sourceMaps.getEmptyMap(this.relativename, this.contents); | ||
} | ||
|
||
if (this.globals.size > 0) { | ||
code = Array.from(this.globals.values()).join('\n') + '\n' + code; | ||
this.sourcemap = this.options.sourcemaps | ||
? sourceMaps.offsetSourceMap(this.sourcemap, this.globals.size) | ||
: undefined; | ||
} | ||
|
||
return { | ||
js: code | ||
js: code, | ||
map: this.sourcemap | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll also need to generate 1:1 source maps for files that we don't process with babel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
}; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
const fs = require('fs'); | ||
const path = require('path'); | ||
const Packager = require('./Packager'); | ||
const urlJoin = require('../utils/urlJoin'); | ||
const lineCounter = require('../utils/textUtils').lineCounter; | ||
|
||
const prelude = { | ||
source: fs | ||
|
@@ -23,6 +25,7 @@ class JSPackager extends Packager { | |
|
||
let preludeCode = this.options.minify ? prelude.minified : prelude.source; | ||
await this.dest.write(preludeCode + '({'); | ||
this.lineOffset = lineCounter(preludeCode); | ||
} | ||
|
||
async addAsset(asset) { | ||
|
@@ -52,7 +55,7 @@ class JSPackager extends Packager { | |
deps[dep.name] = this.dedupe.get(mod.generated.js) || mod.id; | ||
} | ||
} | ||
|
||
this.bundle.addOffset(asset, this.lineOffset, 0); | ||
await this.writeModule(asset.id, asset.generated.js, deps); | ||
} | ||
|
||
|
@@ -64,6 +67,7 @@ class JSPackager extends Packager { | |
wrapped += ']'; | ||
|
||
this.first = false; | ||
this.lineOffset += lineCounter(wrapped) - 1; | ||
await this.dest.write(wrapped); | ||
} | ||
|
||
|
@@ -87,7 +91,15 @@ class JSPackager extends Packager { | |
entry.push(this.bundle.entryAsset.id); | ||
} | ||
|
||
await this.dest.end('},{},' + JSON.stringify(entry) + ')'); | ||
await this.dest.write('},{},' + JSON.stringify(entry) + ')'); | ||
// Add sourcemap url | ||
await this.dest.write( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should only be added if sourcemaps are enabled right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ow yes, will change this |
||
`\n//# sourceMappingURL=${urlJoin( | ||
this.options.publicURL, | ||
path.basename(this.bundle.name).slice(0, -3) + '.map' | ||
)}` | ||
); | ||
await this.dest.end(); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
const path = require('path'); | ||
const sourceMap = require('source-map'); | ||
const Packager = require('./Packager'); | ||
const sourceMapUtils = require('../utils/SourceMaps'); | ||
|
||
class SourcemapPackager extends Packager { | ||
async start() { | ||
this.generator = new sourceMap.SourceMapGenerator({ | ||
file: path.basename(this.bundle.name) | ||
}); | ||
} | ||
|
||
getOffsets(asset) { | ||
let parent = this.bundle.parentBundle; | ||
return parent.offsets.get(asset.relativename) || {line: 0, column: 0}; | ||
} | ||
|
||
async addAsset(asset) { | ||
if (asset.generated.map) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If an asset is added to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably a remain from the initial approach, i'll remove this and test a bit to make sure it doesn't break |
||
this.generator = sourceMapUtils.combineSourceMaps( | ||
asset.generated.map, | ||
this.generator, | ||
this.getOffsets(asset).line | ||
); | ||
} | ||
} | ||
|
||
async writeMap() { | ||
await this.dest.write(this.generator.toString()); | ||
} | ||
|
||
async end() { | ||
await this.writeMap(); | ||
await super.end(); | ||
} | ||
} | ||
|
||
module.exports = SourcemapPackager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we disable sourcemaps in production? Isn't that where they are most useful since code is minified? Let's get some feedback on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think there are usecases for disabling sourcemaps in production. how about enabling it by default and adding an option
--disable-sourcemaps
- so we can choose at build-time?