-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
enable sourcemaps for bundles, include sourcemap for plugins in npm package #24303
Conversation
@XhmikosR As i see, dist/js/bootstrap.bundle.js.map contains all sources, and others also. I don't included recompiled files in commit!!! |
@Delagen: even so, I'll verify after you address the other issue I mentioned above. |
@XhmikosR Which issue? Include recompiled files in commit? |
@Delagen: you haven't enabled sourcemaps for all of our dist JS files. |
@XhmikosR enabled for minified files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need includeSources
and url
for uglify-js. See https://github.com/mishoo/UglifyJS2#cli-source-map-options
Also, we need to do the same for js-minify-docs
.
@XhmikosR updated. Seems includeSources have no effect and enabled by default, but enabled manually. Strange but in source code of UglifyJS it defaults to false. As for me url comment is unneded if file has the same name. If file will be included under different name due user rename, use have to update comment also to make it works. |
@Delagen: |
@XhmikosR strange behavior )) As i see in compiled files
bootstrap.bundle.min.js
|
@Johann-S: any ideas? |
@XhmikosR I understand. Updated |
The sourcemaps still don't show the sources for me. I'll try to find why, otherwise if anyone has any ideas please comment here. EDIT: Hmm, Chrome loads the source files fine. FF v57 beta doesn't here on Windows. |
@XhmikosR I think it's due base&root source map settings. Many |
@Delagen: that is with Chrome, right? Because with Chrome it works for me too but with FF it doesn't. |
@XhmikosR Also in Firefox |
Ah, true. It's grouped under the All right, all seems good now. |
@XhmikosR Yes. The problem is in base and root ) |
@Delagen: do you have a fix for that? |
@XhmikosR I think rollup calculate paths relative to output, so I'll see how to tune this. |
@XhmikosR I think if files required from node_modules, all be fine due long path to it, and it points to real files. If we change source mapping, we broke package debugging, so I think we must keep it unchanged. |
@mdo: are you OK with adding sourcemaps for JS files? I know we didn't include them in v3, not sure what the reasons were. They don't hurt being present plus we already include sourcemaps for CSS files. |
@mdo: ping |
I've no preference on including or not—that's a better question for @Johann-S and @bardiharborow. |
Not against it because we already provide source map for our minified CSS |
@mdo: when you do |
related #19063