-
Notifications
You must be signed in to change notification settings - Fork 15
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
Source Map V3 Support in Node.js #40
Comments
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)
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?
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
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.
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
I'm not sure, but doing so would probably greatly simplify the work Istanbul does today, right?
(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 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 |
As I mentioned in nodejs/user-feedback#59 (comment), 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 I'm not sure why source map support would depend on 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? |
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 To be clear I'm 👍 on node.js having the ability to handle source-maps but I would only want it enabled during testing. |
@SimenB the solution with 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?).
@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
@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 |
I did a bit of programming today, based on conversations so far; curious if folks think this is a step in the right direction:
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). |
@bcoe, I understand what you're trying to do now. Yes, 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! |
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. |
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). |
@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).
@cb1kenobi my perspective is you want both to be observable:
|
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 likets-node
which dynamically insertsource-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 intorequire.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:
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.
The text was updated successfully, but these errors were encountered: