Repository navigation
Support the "sections" alternate source map structure #16
Description
Activity
Seconding this, this feature would be very handy for the grunt-coffee-redux tool.
Proposed API
Goals:
- Not depend on any specific web, node, or Firefox APIs for fetching the sub source maps
- Not force developers using the existing API to change their code. Aside: It's ok if we throw an error when they haven't loaded a sub-source map we need because they wouldn't have been able to consume an indexed source map before and it would have failed anyways.
Notes:
- I'm using
Constructor#methodinstead ofConstructor.prototype.methodbecause the latter is too long and is messing up github's formatting.
SourceMapConsumer#fetchSubSourceMaps(fetcher, callback)fetcher(subSourceMapURL, onSuccess, onFailure): Performs HTTP GET requests to fetch sub source map URLs and passes the information on viaonSuccess(httpResponseBody)oronFailure(error)callbackscallback(sourceMapConsumer, errors): A function called once all of the sub source maps have been loaded. Theerrorsargument should benullin the case of no errors, or a list of error objects if there were errors fetching any of the sub source maps.
If this is not an indexed source map, or every section of this indexed source map is an inline map (not a pointer to a URL) we should call
callbackimmediately.Note that each sub source map could also be an indexed source map, so we will need to make sure to load each sub source map as well before calling
callback.Changes to the
SourceMapConsumerconstructorAdd two optional arguments,
fetcherandcallback(the same as above) so that the function's signature isSourceMapConsumer(sourceMap[, fetcher, callback]).Changes to
SourceMapConsumer#originalPositionFor(generatedPosition)If we are not consuming an indexed source map, proceed normally.
If we are in an indexed source map:
Look up which sub source map the location falls into
If the sub source map is still fetching, throw a "not yet loaded" error
If fetching the sub source map failed, throw a "fetching sub source map for section failed" error
If fetching the sub source map completed successfully:
Determine the relative location into that sub source map by substracting the section's offsets
Query the sub source map for the original position of the relative location and return that.
Changes to
SourceMapConsumer#generatedPositionFor(originalPosition)Note: Because we can't be sure which sub source map contains the mapping with the original position we are being given until we parse each of them, we have to throw an error if any section is not available.
If any of the sub source maps are still loading, throw a "not yet loaded" error
If any of the sub source maps failed to load, throw a "fetching sub source map for section(s) [, , ...]" error
If all the sub source maps have been loaded successfully:
Iterate over each sub source map and ask for the generated position we are looking for
Return the first non-{ line: null, column: null } position we find
If we find none, return { line: null, column: null }
- A
callbackparameter in a constructor seems weird to me - I would prefer node.js callback style for the
fetcherfunction(err, rawSourceMap), but it's ok. - The behaivior for the getters seem weird: They "randomly" throw errors if the map is not fully loaded. There is no chance to get notified if this mapping is ready (only the full map).
- Extending
SourceMapConsumerwith this stuff make the code bigger for everyone. - Staying environement independend is good.
I would like to propose an alternative API for discussion:
It
- doesn't extend the current
SourceMapConsumer - introduce a
AsyncSourceMapConsumerfor nice on demand loading. TheSourceMapConsumerAPI, but async.
var map = new AsyncSourceMapConsumer(rawSourceMap, fetcher); // doesn't fetch anything map.originalPositionFor(genPos, function(err, orgPos) {}); // fetch parts on demand map.generatedPositionFor(orgPos, function(err, genPos) {}); // need to fetch everything map.toSourceMapConsumer(function(sourceMapConsumer, errors) { // gives a sourceMapConsumer in every case. failed mappings are missing. // the errors array summarize all fetch errors. var genPos = sourceMapConsumer.generatedPositionFor(orgPos); // may throw if fetch failed // sourceMapConsumer is a instance of `SourceMapConsumer` or `IndexSourceMapConsumer` });
The class
IndexSourceMapConsumerhas the same API as SourceMapConsumer but work with index maps. It is used byAsyncSourceMapConsumer#toSourceMapConsumer.var indexMap = new IndexSourceMapConsumer(rawSourceMap); var subMaps = indexMap.getSubMapUrls(); // String[] indexMap.setSubMap(url, sourceMapConsumer); indexMap.setSubMapError(url, error); // accessing this sub map will throw that error // same API as SourceMapConsumer
For API users:
- The existing
SourceMapConsumerdoesn't change and cannot consume index maps. - A simple change of the existing code can consume index maps (
AsyncSourceMapConsumer#toSourceMapConsumer) - A complex change of the existing code can consume index maps with on demand loading of source maps (
AsyncSourceMapConsumer#originalPositionForetc.)
Alternative: Merge the
IndexSourceMapConsumerwith theSourceMapConsumer...- Existing code can comsume index maps, if they don't reference other SourceMaps by url.
- But it would increase code size for
SourceMapConsumer.
Alernative 2: Rename
SourceMapConsumertoNormalSourceMapConsumer. Make a newSourceMapConsumerconstructor, which automatically decide betweenNormalSourceMapConsumerandIndexSourceMapConsumer. They are subclasses of the newSourceMapConsumerinterface.- Existing code can comsume index maps, if they don't reference other SourceMaps by url.
- Existing code which subclasses
SourceMapConsumerwill break.
- A
- Definitely agree on preferring node-style callbacks.
- I'd prefer to not introduce another object type for index maps if possible, they're part of the spec & I feel like the
SourceMapConsumerobject should support the entirety of the spec. - I'm not convinced that it will be such a massive amount of code that adding support to the existing
SourceMapConsumeris a problem. That being said, I don't use this code on a client so as long as it's well-tested & well-written code size isn't a pressing concern for me.
Hmm, starting to look into this but the testing code doesn't seem to support async tests. I don't really want to rewrite the test framework, any suggestions @fitzgen? I could wrap the test calls into something like caolan/async but I dunno how kosher that is for @mozilla code.
(Aside: @tivac, @firass just told me that you are also a veteran of ResTek, nice 👍)
- Regarding testing and async: it should be easy to mock a
fetcherthat is not async and just returns hard coded maps depending on the url you give it - Ok, let's do node style callbacks
- I don't see any reason to expose getting a list of the sections or setting a sub source map for a given section in the public API
- Strongly prefer to fetch sections greedily (or at minimum have a switch for this): developers using a debugger don't want to wait around for a subsection of a source map to load because they stepped into a new section of code
- Regarding @tivac's second bullet point: I agree that the public API of a
SourceMapConsumershould support the entirety of the spec, however we should be able to implement that support however we want and if the user gets back anIndexSourceMapConsumerinstance that is still valid. What we shouldn't do is force users to check what kind of source map they have and choose the right consumer, or alternatively check which kind of consumer they get in the end. The user should have just one branch of logic / code path regardless what kind of source map they get.- The existing code path (
new SourceMapConsumer(map)) obviously doesn't support async-ness that we will need when we support fetching sub source maps. I'm ok with maintaining this code path and saying it will only work when nothing is remote. - But we need a new one that supports async fetching and gives us the One Code Path to Rule Them All (works for source maps that don't need remote fetching and source maps that do)
- The existing code path (
- We haven't really discussed generating index maps yet. I think we could just create an
IndexSourceMapGeneratoror even just have aSourceMapGenerator.createIndexMapclass method. The single code path arguments don't apply here because they are two different use cases. - I like @sokra's alternative 2.
- I think that the new
SourceMapConsumershould provide and interface of methods thatNormalSourceMapConsumer(aside/nit: I really don't like "Normal", any better name ideas?) andIndexSourceMapConsumercan implement and they should inherit from the interface so that waylet smc = new SourceMapConsumer(map); smc instanceof SourceMapConsumer // true; - How do we pass in
fetcherto theIndexSourceMapConsumer? As I mention above, I want to avoid having to explicitly check whether I get anIndexSourceMapConsumeror aNormalSourceMapConsumerfrom theSourceMapConsumerconstructor; I should only need one branch of logic. - Regarding sub classing breaking, there are a few options:
- Break sub classing and increment the version to 0.2.0 to indicate a backwards incompatible change. Composition still works fine either way.
- Instead of making
SourceMapConsumeran interface whichNormalSourceMapConsumerandIndexSourceMapConsumerimplement, we could just modify theSourceMapConsumerconstructor to return anIndexSourceMapConsumerinstance when needed. This is moving back towards my original proposal. - SourceMapConsumer's constructor can look at which kind of source map it has, and create the proper "
SourceMapConsumerImplementation" object for the right type of source map it has and then in each of its methods just pass the arguments tothis._implementation.originalPositionForor whatever. This should maintain subclassing, but at the cost of some misdirection and a little bit of code bloat.
- I think that the new
- Regarding testing and async: it should be easy to mock a
Async Tests
I was hoping to test it being actually async, to ensure we were properly handling cases where that is diffcult (out-of-order responses, mostly). If you're cool with it a sync fetcher is ok-but-not-great by me.
Naming
I really prefer something like
SimpleSourceMapConsumeroverNormalSourceMapConsumer, "Normal" implies that index maps are "abnormal". Maybe they are, maybe they aren't. Seems unfair to brand them as such though.SourceMapGenerator support
I'd like to get consuming Index source maps working with an API we're happy with first, generating is also important but at least for me it's definitely secondary. Feeling like we have enough discussion about just consuming that we should focus there first!
Inheritance/API
So long as you're ok with API calls randomly breaking in the async case if they are used before the callback has happened I still see no real strong case for splitting the functionality. If you broke them out
SimpleSourceMapConsumerwould still contain about 80% of the code & I don't know that using inheritance to build anIndexSourceMapConsumerfor the last 20% is really worth it.SourceMapConsumer()could throw an error if it's passed a raw source map that has section elements referencing URLs but not given a fetcher/callback. Is that too obtuse?RE: ResTek: If Answerline was still using YUI2.x when you were there that was totally my fault, sorry! 😳
I was hoping to test it being actually async, to ensure we were properly handling cases where that is diffcult (out-of-order responses, mostly). If you're cool with it a sync fetcher is ok-but-not-great by me.
I agree that it would be best, but I think we can push this back to a follow up issue, or at least start implementing support for index maps now and then merge the async testing in before merging the full feature.
SimpleSourceMapConsumer👍
SourceMapConsumer()could throw an error if it's passed a raw source map that has section elements referencing URLs but not given a fetcher/callback. Is that too obtuse?👍
Inheritance
I think they should be separate. Having
if (this.sections)as the first thing in every method ofSourceMapConsumerwould not be nice.I think they should be separate. Having if (this.sections) as the first thing in every method of SourceMapConsumer would not be nice.
Oof, ok. I can still try to work on it but I don't think I'll have enough time for a refactor that large. We'll see what I can accomplish.
Just another idea: We could give methods like
originalPositionForan optional callback. In the sync version they throw not-yet-loaded Error. With callback they return, when the required part is loaded. This would keep the API backward compatible, but give new code the opportunity to react to the fetching process.New methods include:
fetchSubMaps([fetcher], [callback])(greedily) andsetSubMapFetcher(fetcher)(on demand) withfetcherbe afunction(url, callback(err, rawSourceMapObject))I really like that idea, but it doesn't work with
SourceMapConsumer.prototype.sourcesbecause it's a getter._sigh_
What would be the prefect API if we didn't worry about backwards compatibility? Maybe its time...
That's a good question... Maybe something like this:
interface SourceMap { // Personally I would rename it^^ // callback is "function(err, result: X)" for "X abc([callback])" {source,line,column,name} orginalPositionFor(line, column, [callback]) {line,column} generatedPositionFor(source, line, column, [callback]) String[] allSources([callback]) {source: sourceContent} allSourceContents([callback]) // Mapping is {original:{source,line,column,name},generated:{line,column}} Mapping[] allMappings([order], [callback]) Mapping[] mappingsFor(source, [order], [callback]) Mapping[] mappingsFor(startLine, startColumn, endLine, endColumn, [order], [callback]) } class SimpleSourceMap : SourceMap { new SimpleSourceMap(sourceMap: Object) } class IndexSourceMap : SourceMap { new IndexSourceMap(sourceMap: Object, [fetcher]) fetch([callback]) } class FutureSourceMap : SourceMap { new FutureSourceMap(url: String, fetcher) fetch([callback]) } module "source-map" { // callback is "function(error, sourceMap: SourceMap)" static SourceMap create(sourceMap: Object, [fetcher, [callback]]) static SourceMap create(sourceMap: Object, [fetcher, fetchLazily: true]) static FutureSourceMap create(url: String, fetcher, [callback]) static FutureSourceMap create(url: String, fetcher, fetchLazily: true) }
Just another idea: We could give methods like originalPositionFor an optional callback. In the sync version they throw not-yet-loaded Error. With callback they return, when the required part is loaded. This would keep the API backward compatible, but give new code the opportunity to react to the fetching process.
New methods include: fetchSubMaps([fetcher], [callback]) (greedily) and setSubMapFetcher(fetcher) (on demand) with fetcher be a function(url, callback(err, rawSourceMapObject))
Let's combine this proposal and the
this._implementation.originalPositionForso that inheritance doesn't break.We can change
sourcesto be a method with an optional callback and bump the version number.What's the current status of this?
It looks like #69 is in limbo at the moment.
A lot of this discussion seems to revolve around asynchronicity, but having a subset of this functionality synchronously would be really useful to me.
For my current project, I need to be able to consume indexed source maps, but each of the
sectionshas amapinstead of aurlin it (i.e. once the map is loaded, no more fetching is required).For example:
{"version": 3, "file": "foo.out", "sections": [ {'offset': {'line': 0, 'column': 0}, 'map': {"version": 3, "file": "i1", "sourceRoot": "/", "sources": ["i1"], "names": [], "mappings": "AAAA"}, }, {'offset': {'line': 1, 'column': 0}, 'map': {"version": 3, "file": "i2", "sourceRoot": "/", "sources": ["i2"], "names": [], "mappings": "AAAA"}, } ]}- Would you be open to accepting a patch that provides that functionality without adding asynchronicity?
- After all the discussion about potential APIs, I'm unsure what the final proposal is.
Would you be open to accepting a patch that provides that functionality without adding asynchronicity?
Yes.
After all the discussion about potential APIs, I'm unsure what the final proposal is.
Add a method for setting the source map fetcher. It should take
(urlString, callback)and call the callback with(error, null)on a fetch failure, or(null, data)on fetch success.Add a method to greedily fetch all sub source maps that takes a callback which gets passed
(error)on failure or(null)on success.All methods that access source mapped data should take an optional callback. If no callback is provided and the data is available, return it. If no callback is provided and the data is not available, throw an error. If a callback is provided and the data is available, call the callback with the arguments
(null, data). If a callback is provided and the data is not available, attempt to load it. If it fails to load, call the callback with(error, null). If loading succeeds, call the callback with(null, data).Make
sourcesa function (taking an optional callback, as above), not a getter.Pushing two years on this... what's that hard part about recursively iterating a sourcemap?
Pushing two years on this... what's that hard part about recursively iterating a sourcemap?
Sounds like you're volunteering your time! Great! When should we expect your contributions?
Reacted by Ben VinegarAfter I've had time to fully parse and understand the commentary in this thread, as well as source-map-consumer.js. So... indefinite.
- added 2 commits that reference this issue
on Jul 20, 2015 Wow, it would be really awesome to have this so we could have source maps for HTML files with inline scripts on different parts of a document, especially web components. Can I please just confirm if the spec proposes this but it has not been used or implemented yet?
Thanks guys- added a commit that references this issue
on Oct 15, 2017 Wow, it would be really awesome to have this so we could have source maps for HTML files with inline scripts on different parts of a document, especially web components. Can I please just confirm if the spec proposes this but it has not been used or implemented yet?
Inline
<script>can set the source map URL (and thesourceURLif desired). This can work fine without needing the sections approach. However, there are still some bugs in this area in Firefox at least. I'm not totally sure this answers your question...?Thanks, @tromey!
I actually meant when you have multiple<script>tags in a single HTML file since only onesourceMappingURLdeclaration is accepted per file. I actually did some tests yesterday and could get it working well in Chrome with sections defined, which was great 😄
Perhaps that's already good enough because when I first saw this issue opened I wondered if thesectionfeature was actually implemented or just a proposal.I actually meant when you have multiple <script> tags in a single HTML file since only one sourceMappingURL declaration is accepted per file.
I think the intent is that each separate
<script>can have its ownsourceMappingURL; but like I mentioned, there are some bugs in the Firefox devtools in this area. I don't think source-mapping the entire HTML document is supported (if it is, that must be some browser's own extension).I wondered if the section feature was actually implemented or just a proposal.
I think it is implemented; however, in this library, there's no support for a sectioned source map where the sections are not inlined. Nobody ever implemented the proposed fetching API.
- added a commit that references this issue
on Oct 19, 2017
https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit?pli=1#heading=h.535es3xeprgt