First stab at classes #336
Conversation
… the API the same. That file needs careful review. Everything else is pretty straightforward. Fixes mozilla#334. Fixes mozilla#259.
| @@ -5,116 +5,96 @@ | |||
| * http://opensource.org/licenses/BSD-3-Clause | |||
| */ | |||
|
|
|||
| const util = require("./util"); | |||
| const has = Object.prototype.hasOwnProperty; | |||
| const hasNativeMap = typeof Map !== "undefined"; | |||
There was a problem hiding this comment.
Everywhere that has classes has Map. No need for this backward-compat anymore.
| * | ||
| * @returns Number | ||
| */ | ||
| size() { |
There was a problem hiding this comment.
Note to self: next time don't reorder functions at the same time. Yes, it's nice to have the static's at the top, but it makes the patch harder to read.
|
|
||
| class SourceMapConsumer { | ||
| constructor(aSourceMap, aSourceMapURL) { | ||
| // If the constructor was called by super(), just return Promise<this>. |
There was a problem hiding this comment.
Is this enough comment for the hack?
lib/source-map-consumer.js
Outdated
| sourceMap = util.parseSourceMapInput(aSourceMap); | ||
| } | ||
|
|
||
| return Promise.resolve().then(_ => { |
There was a problem hiding this comment.
There's no reason to put a then() here. We can just put the trigraph inside resolve().
| }); | ||
| } | ||
| static initialize(opts) { | ||
| readWasm.initialize(opts["lib/mappings.wasm"]); |
There was a problem hiding this comment.
This line isn't covered in the tests. Does running the tests in the browser depend on #158 ?
There was a problem hiding this comment.
This line isn't covered in the tests.
Yeah, that's because it isn't needed on node.
Does running the tests in the browser depend on #158 ?
I don't think so... Unsure if that issue is still relevant at all or not.
lib/source-map-consumer.js
Outdated
| } | ||
| class BasicSourceMapConsumer extends SourceMapConsumer { | ||
| constructor(aSourceMap, aSourceMapURL) { | ||
| return super(INTERNAL).then(that => { |
There was a problem hiding this comment.
You thought that was a holdover from before arrow functions. But in this case, the constructor returns a Promise, which is pretty non-standard. So hence the workaround.
| /** | ||
| * The version of the source mapping spec that we are consuming. | ||
| */ | ||
| BasicSourceMapConsumer.prototype._version = 3; |
There was a problem hiding this comment.
Used the one off the base class prototype. Don't know if that's strictly what was desired, but this whole conglomeration of classes is unlikely to work for another version number than 3.
| @@ -5,116 +5,96 @@ | |||
| * http://opensource.org/licenses/BSD-3-Clause | |||
| */ | |||
|
|
|||
| const util = require("./util"); | |||
| const has = Object.prototype.hasOwnProperty; | |||
| const hasNativeMap = typeof Map !== "undefined"; | |||
| }); | ||
| } | ||
| static initialize(opts) { | ||
| readWasm.initialize(opts["lib/mappings.wasm"]); |
There was a problem hiding this comment.
This line isn't covered in the tests.
Yeah, that's because it isn't needed on node.
Does running the tests in the browser depend on #158 ?
I don't think so... Unsure if that issue is still relevant at all or not.
|
@fitzgen: ready to merge, as far as I can tell. |
|
Thanks! |
source-map-consumer.jsis a mess in order to keep the API the same. That file needs careful review. Everything else is pretty straightforward. Fixes #334. Fixes #259.Don't merge yet; I want to double-check the coverage and think about it a bit more. But I'd love a few extra eyeballs on this in the meantime. Feel free to be brutal in comments.