Source Map V3 Support in Node.js

bcoe commented 5 years ago

There's an open PR to start caching source-maps when scripts are loaded in Node.js; It piggybacks off the NODE_V8_COVERAGE implementation, and is an attempt to solve problems presented by tools like ts-node which dynamically insert source-maps at runtime.

I've been scratching my head and trying to think of what a more generic solution looks like for the community...

Is the additional functionality added to `NODE_V8_COVERAGE` too much of a hack?

This would allow upstream tooling to start working for tools like esm, ts-node, ts-mocha, without any additional work, e.g., hooking into require.extensions, application exit events, but, is it adding something into Node.js we'll regret?

Is it useful for Node.js to maintain and expose a cache of loaded source-maps

An alternative would be the folks override require.extensions and eventually loader-hooks for ESM, and detect source-maps themselves in userland ... this is how things work today.

If we do maintain a cache, what should an API for interacting with it look like?

I was picturing something like this:

const {fetchSourceMap} = require('source_map');
const sm1 = fetchSourceMap(require.resolve('./my-script'));

Should Node.js rewrite stack traces, taking into account source-maps?

Today there's a popular tool node-source-map-support that does this, if Node.js was now maintaining a cache of source-maps, should we make this a first-class-citizen of Node.

Alternatively, we could try to upstream this behavior into V8, but this might be a fairly long-term project.

Should Node.js also provide an API for applying with source-maps?

If Node.js were to apply source-maps to stack-traces, we would already need to ship something like source-map for parsing and applying the maps, should this API be exposed?

What's the MVP functionality that would be useful to folks in the community?

tldr; I've looped in a few folks who work on Source Map adjacent projects, and would love to know people's opinions ... what's an MVP that would make people's lives easier?

CC: @evanw, @LinusU, @cpojer, @SimenB, @joyeecheung, @boneskull, @iansu, @jdalton.

SimenB commented 5 years ago

This is super exciting, thank you so much for working on it! 🎉

(For background on my thoughts below, I'm one of the maintainers of Jest)

Is the additional functionality added to NODE_V8_COVERAGE too much of a hack?

I'm not in a position to have an opinion here. Only comment is that in my (WIP) implementation of V8 coverage in Jest, that env variable is not really useful. However, if it's a stepping stone for getting support for source maps natively into Node it seems pretty natural to group it under the thing that prompted its inclusion until source maps potentially are regarded as stable?

Is it useful for Node.js to maintain and expose a cache of loaded source-maps

I haven't really thought it through yet, but the main thing coming to mind is that a single file might be transformed in different ways within the same process (e.g. with and without coverage, or both for es6 and es5), so a cache based on file paths wouldn't necessarily work. If the fetchSourceMap you mention above just takes a unique string though (and not just a file path), that's not an issue. Also, it would need to be accessible to child processes and worker threads, which might pose an issue if the cache is kept in memory?

If we do maintain a cache, what should an API for interacting with it look like?

I touched on it above, but (at least for Jest's case) a file path is not necessarily unique enough, but if the argument is just a string I think that API can work a treat.

Should Node.js rewrite stack traces, taking into account source-maps?

Yes please, that would be amazing! We've had memory issues in Jest due to source maps, and I imagine a solution within node itself could help mitigate this (e.g. by being able to use the wasm implementation of source-map in a synchronous matter via loading it earlier/in native code)

Should Node.js also provide an API for applying with source-maps?

I'm not sure, but doing so would probably greatly simplify the work Istanbul does today, right?

what's an MVP that would make people's lives easier?

(This is again from Jest's perspective)

I think just caching it doesn't really change anything as that's the simple part (or rather, the heavy lifting is already done in a few different modules). Actually applying a source map to some transpiled code is harder, so I think some way of rewriting stack traces (like the linked node-source-map-support does) would be a great start that is relatively small in scope while still needing real source map support.

Like I've said a few times before, thank you so much for driving this. 👏 It's a super exciting development that I think can really make creating great DX easier

cb1kenobi commented 5 years ago

As I mentioned in https://github.com/nodejs/user-feedback/issues/59#issuecomment-396332169, I'm a huge +1 on this feature!

All of our packages that have transpilation use source-map-support. We inline source maps and we call register from the index.js. It works, but I wish Node had this built-in which would A) allow us to no longer depend on a ~900KB dependency and B) likely give us a tiny performance boost.

I'm not sure why source map support would depend on NODE_V8_COVERAGE. I would prefer Node always detects the existence of a source map and use it in stack traces automatically.

As far as caching is concerned, our source maps are inlined and I presume the caching you're talking about is when the maps are external. We use inlined source maps because there was some issue with external source maps like 4 years ago with I first tinkered with Babel and NYC. I'd be curious what the benefits of an external source map would be. Better performance?

coreyfarrell commented 5 years ago

I'm not sure why source map support would depend on NODE_V8_COVERAGE. I would prefer Node always detects the existence of a source map and use it in stack traces automatically.

I worry about the memory usage and startup/module load delay for 'always on' source maps. I'm not sure it should ultimately depend on NODE_V8_COVERAGE but I think it should somehow be opt-in.

To be clear I'm :+1: on node.js having the ability to handle source-maps but I would only want it enabled during testing.

bcoe commented 5 years ago

It would need to be accessible to child processes and worker threads, which might pose an issue if the cache is kept in memory?

@SimenB the solution with NODE_V8_COVERAGE was to output coverage information to disk (this is similar to what nyc does as well); this puts us in the position to reassemble information from a variety of threads and workers.

I thought your thinking with Jest, was that you would collect coverage information through a long-lived inspector session? wouldn't this be isolated in one Node process where an in memory cache of source-maps could be useful?

Short of a cache on disk, I'm trying to picture what you're thinking? (is there a similar Node.js API that's analogous?).

Yes please, that would be amazing! We've had memory issues in Jest due to source maps, and I imagine a solution within node itself could help mitigate this

@SimenB interesting, would love to know the specific memory issues, sounds like something we should mindful of (if we're taking the naive approach of just shipping source-map).

I was actually concerned about the wasm implementation, because I believe it's an async API, and process.exit needs to happen in one tick of the event loop (we can't really have the source-map API be async). Curious, @loganfsmyth, do you have any thoughts on this topic; If we were going to add source-map support into Node.js, what library should we ship?

I'm not sure why source map support would depend on NODE_V8_COVERAGE. I would prefer Node always detects the existence of a source map and use it in stack traces automatically.

@cb1kenobi, I think we're thinking of two different problems here; I'd like to solve the use-case of upstream tooling (I specifically work on source-maps) wanting to apply source-maps, so that they can display reports that reference the original source -- I'm 100% in agreement that we should do something automatically with stack traces.

@cb1kenobi, you mention in your other thread that source-map-support uses prepareStackTrace; I noticed that @devsnek did some work to move this API into Node.js last year (@devsnek if --collect-source-map was enabled, could we just override prepareStackTrace in Node.js providing better context for transpiled code?).

bcoe commented 5 years ago

I did a bit of programming today, based on conversations so far; curious if folks think this is a step in the right direction:

we introduce the flag --experimental-source-maps, which if flipped on starts collecting source-maps in a cache (the cache design can be seen here).
if an error occurs, and --experimental-source-maps is set, stack traces are annotated as follows:

Error: goodbye
    at  /Users/bencoe/oss/node-1/test/fixtures/source-map/inline-throw.min.js:1:43
         -> /Users/bencoe/oss/node-1/test/fixtures/source-map/throw.js:8:2
    at  Immediate.<anonymous> (/Users/bencoe/oss/node-1/test/fixtures/source-map/inline-throw.min.js:1:60)
         -> /Users/bencoe/oss/node-1/test/fixtures/source-map/throw.js:8:2
    at  processImmediate (internal/timers.js:439:21)

I have a working proof of concept of this here.

@cb1kenobi, @SimenB, does this seem like a step in the right direction?

@SimenB in your case it doesn't give you an API for Jest as of yet, but I think perhaps this is a step in the right direction (I'm excited that it ports SourceMap.js from V8, which is a lightweight implementation of the Source Map V3 API, we could expose this eventually).

cb1kenobi commented 5 years ago

@bcoe, I understand what you're trying to do now.

Yes, prepareStackTrace sounds like a reasonable place to apply source maps to a stack trace.

In regards to your last comment, for what I work on, I don't see the benefit in knowing the line number in the minified source file. I'd rather have a concise stack trace that has the source mapped line numbers, but I'm fine with either way. I appreciate you spearheading this effort!

ljharb commented 5 years ago

Note, though, that the stack trace language proposal will eventually require that stack traces be observably “prepared” at error creation time, and no “prepareStackTrace” method can exist for that purpose - it would be ideal for node to avoid attaching to it if possible.

devsnek commented 5 years ago

oopsie I missed this ping. node internals use a special c++ api I added to V8, not the "public" prepareStackTrace function (in fact, we have to polyfill prepareStackTrace since the c++ api replaces it).

bcoe commented 5 years ago

node internals use a special c++ api I added to V8, not the "public" prepareStackTrace function

@devsnek, so with regards to @ljharb's comments; it sounds like we're relatively future-proof for the time being? I can imagine eventually moving this behavior into V8, when we have a design document, etc., (but I'm guessing it's a ways out before their team has cycles, based on talks with Yang).

In regards to your last comment, for what I work on, I don't see the benefit in knowing the line number in the minified source file. I'd rather have a concise stack trace that has the source mapped line numbers

@cb1kenobi my perspective is you want both to be observable:

some folks ship source-maps without the original source-code, wo you could end up with stack-traces in node_modules/ that don't map back to an actual file.
there's always a chance that an exception occurs in the transpiled code but not the original.

nodejs / tooling