Replace `--enable-source-maps` with `--disable-source-maps`
brillout opened this issue · 16 comments
How about Node.js sets --enable-source-maps to true by default. If the user wants to get the real stack track, he can use --disable-source-maps.
The problem here is that tools like Vite cannot really leverage --enable-source-maps because it forces Vite users to set the flag --enable-source-maps which is less than ideal. We don't want to tell Vite users "you need to use this and that flag"; we want things to just work. We are actually interested in using source map support (which is a lovely new feature!) but the flag thing is a problem, see related discussions at vitejs/vite#3300.
Source map support is mostly taken care by frameworks; it is rarely implemented by the Node.js user himself. So we shouldn't leak this flag into user-land.
Also, in general, I'd argue that having source maps enabled by default seems to be a more sensible default.
Runtime source maps come with performance penalties. I can hardly believe it will be acceptable to enable source maps by default even in server production environments.
Makes sense. Maybe default to true in dev only then, by checking the NODE_ENV env var. We can keep the --enable-source-map flag for production.
Does Node itself currently use the NODE_ENV to control anything? My impression was that this was merely a convention among package managers and packages, albeit a widely-used one. I think it would be weird to start checking NODE_ENV within Node for this one thing.
I can hardly believe it will be acceptable to enable source maps by default even in server production environments.
This could be argued either way as to whether proper stack traces should be captured or not by default (even in production environments). Presumably this only has any effect if a sourcemap exists, so it seems likely those who bother to ship sourcemaps to production would want them to apply (otherwise they could just not bother shipping them).
Runtime source maps come with performance penalties.
Really the thing that makes it questionable as to whether it could be default or not is when do these performance penalties actually apply.
- Does it significantly impact startup time?
- Does it cost when throwing an error or only when
.stackis read? - Does it have impacts on files that have source maps but never throw errors?
- Does it affect any other APIs other than
Error.prototype.stack/Error.captureStackTrace? - Does it affect files that don't declare a sourcemap?
In my opinion it would be fine to enable by default if it only applies a performance impact in cases where one would actually require it to, i.e. in files that use //# sourceMappingURL, only when stacks are actually captured, and no performance impacts at other stages (e.g. during startup).
Does it significantly impact startup time?
It can impact startup time. Node.js parses source-maps on loading the JavaScript sources so that would take times.
Does it cost when throwing an error or only when
.stackis read?
It costs only when .stack is read.
Does it have impacts on files that have source maps but never throw errors?
Like said above, it can impact startup time if the source maps are unreasonably large.
Does it affect any other APIs other than
Error.prototype.stack/Error.captureStackTrace?
Anything that printing errors will be affected as they are accessing Error.prototype.stack, like console or util.format (which is heavily used in common loggers).
Does it affect files that don't declare a sourcemap?
Every line in Error.prototype.stack will be iterated to see if the frame is known to loaded source maps. So the whole program would take the performance penalties.
it would be fine to enable by default if it only applies a performance impact in cases where one would actually require it to, i.e. in files that use
//# sourceMappingURL
No, errors thrown in any file will be penalized on accessing error.stack even if they themselves don't declare a source map.
I cannot say that the impact is significant or not, as it may vary in different use cases. But small reduction can be accumulated to cause an observable slow down as source maps in node_modules must be counted.
However, I believe that a programming API that enables built-in source-maps support seems more reasonable to the OP's requirement. Vite or any developer tools can determine if they would like to enable source-map-support in that process at the very start of the program. In this way, there will be no need for users to set the --enable-source-maps flags for vite. Nor do we need to enable it by default in every environment.
If a programming API sounds reasonable to you, I believe it will be a good start to work in that path.
If a programming API sounds reasonable to you, I believe it will be a good start to work in that path.
Yes, that sounds quite perfect actually.
So Vite will be able to decide whether source maps are applied? Meaning Vite could expose a flag vite.config.js#disableSourceMap that Vite users can use?
Btw. is there a way to access the original stack trace? Source maps are often broken and not being able to access the exact source of error is extremely painful. When the source mapping is broken, it'd be wonderful to be able to dig into the original stack trace and do the mapping myself then having no clue where the error originates.
Btw. is there a way to access the original stack trace?
Sorry, but I didn't recall any means to do that without colliding the functionality provided by --enable-source-maps after 88d9268 has landed.
Source maps are often broken
May I have your elaboration on this? It will be helpful for us to resolve the problem if it could be fixed in Node.js core.
May I have your elaboration on this? It will be helpful for us to resolve the problem if it could be fixed in Node.js core.
I meant when the source map data itself is wrong, which is frequently the case.
That's why being able to access the original stack trace is important for these situations when the source mapping is wrong.
Debugging a broken stack trace is super painful.
Does it significantly impact startup time?
It can impact startup time. Node.js parses source-maps on loading the JavaScript sources so that would take times.
Does it impact startup time when there are no source maps?
(If not, then most existing code's startup time won't be affected, right?)
Anything that printing errors will be affected as they are accessing
Error.prototype.stack, likeconsoleorutil.format(which is heavily used in common loggers).
In what situations would a developer/user reading Error.prototype.stack want speed instead of legibility?
(As a devops person, I expect Error.prototype.stack to be slow. I only use it after an unhandled error has been thrown. Are there other notable use cases?)
However, I believe that a programming API that enables built-in source-maps support seems more reasonable to the OP's requirement.
Personally, I'm interested in the default. Moving to Typescript, I now gripe that I must add --enable-source-maps to every Docker image I ever create, forevermore. I expect virtually everyone using Typescript has the same issue.
This could be argued either way as to whether proper stack traces should be captured or not by default (even in production environments). Presumably this only has any effect if a sourcemap exists, so it seems likely those who bother to ship sourcemaps to production would want them to apply (otherwise they could just not bother shipping them).
This reasoning is convincing. Don't ship the sourcemaps, and they won't get used.
Source maps are often broken
May I have your elaboration on this? It will be helpful for us to resolve the problem if it could be fixed in Node.js core.
@legendecas I'm currently developing a website using Gatsby. I'm seeing an exception thrown like this:
at actions.createParentChildLink (/SNIP/node_modules/gatsby/src/redux/actions/public.js:1010:24)
However, no file exists under this name. Only node_modules/gatsby/dist/redux/actions/public.js exists. And line 1010 is incorrect too. The source maps are probably usable for Gatsby developers who work on this code, and have created a link from src to their working copy. And while useful in that particular situation, this source map is indeed broken. As an end user of the package, the stacktraces are meaningless for me, and make it more difficult for me to find the place to put a breakpoint for debugging.
see: #41541 for why I think it would be a bad idea to enable source maps by default.
I think we'll gradually get source maps more performant over time, but people would shoot themselves in the foot if we enabled them by default.
There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.
bad bot
There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.
bad bot
I'll close this then. Bumping is annoying and the consensus seems to be we're not going to enable this by default for performance reasons.