-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Is enabling full source maps in production a wise default? #769
Comments
One other benefit of moving to a more restricted source code default would be shorter asset precompile build times. I noticed this was enabled by default when I saw source map compilation took a few seconds on a relatively small app. |
Sourcemaps are super useful for error reporting, as well as for analysing bundle size from dependencies. Whether one chooses to deploy them or not is their choice, but producing them is useful. Producing proper stack traces is another thing to consider in production with things like Rollbar or Sentry. Please note, source maps are only downloaded if devtool is open, so the normal user won't see it unless someone opens the devtool. In terms of exposing the source code, I guess an uglified js file can be easily beautified :) If you wish to remove sourcemaps though, this is how you can do it: // config/webpack/production.js
const merge = require('webpack-merge')
const environment = require('./environment')
const config = environment.toWebpackConfig()
config.devtool = 'none'
module.exports = config |
I don't disagree at all that sourcemaps are useful. I only opened this issue because it's a surprising default to enable it in production, for these reasons:
The more I've thought about this, the more I feel that a more limited setting like |
Thanks, @searls! I think |
@searls Thanks, makes sense 👍 |
This will generate source maps only to the extent required for stack trace reporting tools like Sentry and Rollbar. It should result in build time savings and prevent people's unobfuscated source code from "leaking" with every production build. Fixes rails#769
Sorry to comment on a closed issue - but how do I actually enable source maps now, for tools like HoneyBadger? Any tips? Thank you! |
@ezuk ^_^ // config/webpack/production.js
const merge = require('webpack-merge')
const environment = require('./environment')
const config = environment.toWebpackConfig()
config.devtool = 'sourcemap'
module.exports = config |
I was just made aware of this thread, and would like us to revert. I fully understand and respect the reasons for why some might feel the necessity to obfuscate their code, but I don't think it's a good default, and I don't think it sends a good message. I've learned so much from View Source over the years. From structure, to CSS, to JS. That has gotten harder with minification and transpiling. That's a real shame. The web has given us so much. The default should be to give back, in all the ways we can. View Source is a heritage feature that a framework like Rails should go out of its way to pay tribute to. So. I'd like the source-maps-by-default restored, but with an easy and documented out for people who don't feel they can or will pay tribute to the web in this fashion. Thanks! |
Source maps are back: #1918 I'm going to write now how to disable them in docs. |
@duaneking Are you implying that startups are going to be made or broken by whether source maps, which are only downloaded when development tools are open, are turned on by default? And that they'd somehow be oblivious to this fact right up until their death, and then they'd go, "oh fuck, IT WAS THE SOURCE MAPS THAT DID US IN!!!" 😄. I don't find that to be a major threat for Rails to be concerned about. And certainly not compared to the gains for allowing developers of said startups to learn from other Rails apps that'll ship source maps by default. I appreciate your time to raise the point, though. We'll make it clear in the documentation how you can turn this off, if you don't want to help others learn from view source, in the deep tradition of the web. But the default stays. |
FWIW, I support David's decision here. As long as it's well-documented and easy to disable, having the web be more accessible by default is something that Rails can (and has, numerous times) positively influence. I think I'd need to see some hard data that supports the argument that sourcemaps are somehow a high-cost burden. While very large projects could see longer build times when source maps are enabled, that probably won't be the case for startups on shoestring budgets. As for perceived bandwidth cost, I believe it's the case that all browsers that support loading sourcemaps will only fetch them if the user opens dev tools, which is probably an infinitesimally small number. |
I just wanted to voice my support for bringing this back by @dhh . We have been using source maps at Discourse now for 4 or so years, including maps for both JS and SCSS in production, default on. One non-obvious but super important reason to enable this is that often JS frameworks have "production" and "development" modes. I have seen many cases over the years where a particular issue only happens in production and does not happen in development. Being able to debug properly in production is a huge life saver. Source maps are not the panacea as they still have some limitations around local var unmangling and other edge cases, but they are 100 times better than working through obfuscated minified code with magic formatting enabled. The only legitimate possible performance concern is the cost of precompliation cause in theory this can add some time to precompilation that in turn slows down deploy cycles. From my experience at Discourse the cost has been minimal, that said I do not know what the cost is if you are chaining an enormous amount of source maps all the way down to 10,000 But... before people freak out here about the rising cost of pre-compliation we got to get some real world numbers. |
If bandwidth is a real concern we can gzip source maps just like we do with other assets:
I'm going to enable |
@dhh You say
but this doesn’t jibe with:
So for the past 7+ years, the default in Rails, per your own change, has been to obfuscate code. Why has Rails not gone “out of its way to pay tribute” to this behavior anytime in these many years? |
@amarshall I see that as a change of heart and something to be praised not criticized. Just because someone took a "less than ideal" decision 7 years ago doesn't mean they can't change their minds and make a better decision in the future. Also, we're not taking into account the massive changes Rails has seen since 3.2… JS had a VERY different proposition and role back then compared to now. It used to be just "sprinkles" and little to no logic. I believe this change of heart has a lot more to do with how much more logic is going into JS. Back in Rails 3.2, you could very easily "deobfuscate" CSS and JS locally. It's become increasingly difficult, hence this decision making a lot of sense now. Not trying to speak for anyone, but I definitely applaud this decision. |
compression/uglification's purpose isn't mainly to just obfuscate code, but to shrink the size. The reasoning behind wanting to compress/uglify is important. |
@aequasi that's true but compression/uglification isn't affected by source maps, you can have both simultaneously. |
I’m just pointing out that this philosophy is new for Rails. That’s okay; change is okay. But it should be a considered, reasoned change with more than just one point of consideration (that obfuscation is bad). Rails also espouses convention over configuration, and since it is the convention of the wider JavaScript community and the recommendation of the library which this gem wraps to not provide source maps in production, deviation not just from what one has come to expect from Rails over many years, but also from the larger JavaScript ecosystem, should not be done lightly. @levifig On the other side of “more logic is going into JS” is that technical performance considerations may have a greater impact than before due to the increased amount of code being compiled and served. @aequasi Yes, but the point of excluding source maps is not necessarily just to obfuscate either, as there is potentially a performance impact. As Sam said above “we got to get some real world numbers”. My own opinion is that Rails should stick with the wider community’s convention and the recommendation of Webpack (thus also adhering to the principle of least surprise). That is, until there is evidence that there is a negligible performance/bandwidth impact both in the browser and during compilation. |
We can add a warning
|
Question: If the docs on how to change the source maps is so well documented, why is it necessary to also change the default? It seems like it's just an opinion that there are valid arguments for/against on both sides, but ultimately not a really impactful change. |
Because defaults matter, and the Rails default will be the one paying tribute to the web. |
Anecdotally, @dhh & @javan were pretty cool with me stumbling over stimulus.js in the basecamp source code before it was officially released. I was also able to send the team detailed info about a basecamp JS error. (beneficial for both developer and consumer) IMHO, if you want to run code in someone's browser, don't try and obfuscate it. If you are exposing |
in a high traffic prod site — especially one with a lot of JS— the benefits of having source maps in prod for error reporting alone outweight the downsides. interaction issues, 3rd party code that runs only on prod and causes conflict with your code, etc, etc. ironically, if you run little JS then the source maps in prod make less sense for you, but if you’re app is bloated with JS you will definitely want to see them. would you throw away your back-end exceptions like you just don’t care that users are hitting bugs? I doubt it. |
@jasonfb you can retroactively apply source maps to a file 🙂 it's not a necessary step to compile them in order to deploy to production, so best not to conflate the two processes. In any case, it's an opinion and there's no one right answer, hence my previous question about why you'd change it at all. You might see them as very useful, but there are very legitimate reasons to not use them. |
I agree to keep it enabled by default! @dhh tribute to the web :) |
As someone who has spent a lot of time with Rails 4 apps, and that's where I started learning, I just want to put in a vote for how much default source maps would have helped me! IMO, source maps are 🔋s included -- install an exception tracking gem of choice, Looking forward to this. |
Sorry for posting on a closed topic, or if this seems like a shameless advertisement for my own website, but I explored the topic of production source maps extensively in a blog post and provided a solution which works really well. Essentially if you're worried about security, you can just use any form of authentication to lock down the endpoints your source maps are available at. |
Not interested in any of that. The point about is EXPLICITLY to let "the public" read the source code that runs in their browser. Also, if the security of your applications relies on the obfuscation provided by uglification or minification, I'd argue that's not much security at all. |
@davidomid It's like going into a restaurant where you can see the big open kitchen and stacks of ingredients. You show that you are proud of the scripts you are loading in your client's browser. |
Last comment on here for me, but @jakeNiemiec I'm not proud of the scripts I am loading into my client's browser; I'm proud of the product I'm delivering to them and the use-case it solves. From my point of view, this is a purely technical tradeoff: do you want to increase your compile times and transfer times so you can look at stack traces and source-maps in production? I feel like anything more than that is more of an emotional/personal decision or irrelevant (see: security through obfuscation). To me, for the average user, this just seems like a misuse of transferred bytes for the sake of our own vanity/to pay tribute to the web. I doubt when a car is put together it's done so in a way that lets the consumer easily take it apart. |
I agree that there's plenty of arguments and good cases to be made that each site is unique. I think a lot of high-traffic websites these days run LOTS of Javascript — like hundreds and hundreds of external servers loading Javascript (not that I think this a good thing, and of course I acknowledge that if it is external js that is not relevant to the sourcemap that comes from Rails). My point is just: if that's you, not looking at your JS errors is just as bad as not looking at your Ruby exceptions, and I hope we'd all agree that you want to see your Ruby exceptions. If your site has very little Javascript and you don't need to see errors in prod by all means skip the sourcemaps. |
There are situations where we want to easily share code with others and situations where we don't. Setting a default that overrides the usefulness of other plugins is making a strong statement. (ie UglifyJs) It should be clear that such a statement is being made. |
@swirlingsand Remember that |
Guys, enjoing the issue above, i would like to put a personal project mine to the github. |
This ticket is about the default production configuration (both the webpack & uglify settings) in this file:
webpacker/package/environments/production.js
Line 30 in 50aaf77
In my experience in (mostly Node.js-based) static asset compilation ecosystems, I haven't run across very many cases where full source maps are, by default, generated and pushed to production servers. I presume the reason for this is because that modern JS build toolchains provide a sort of accidental benefit of code obfuscation, and well-generated source maps can actually make it very easy to rip off someone else's source.
The webpack docs don't actually justify this statement, but they say this much about using the
source-map
setting in production:I've looked through the repo and was curious if there was much discussion/justification for enabling full source-maps in production by default and whether it'd be fair to revisit the issue in a thread here.
Curious to hear your thoughts & thanks for your time! ✨
The text was updated successfully, but these errors were encountered: