-
Notifications
You must be signed in to change notification settings - Fork 29
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
Make sourcesContent
optional
#125
Comments
Hmm... this is an interesting concept. There would have to be a way to also set these two for any individual compiler in case people might want different settings. So I would start with adding them as individual compiler options (which can still be implemented in one place through the class they all inherit from) then we can talk about universal options 😀 |
I just stumbled across this issue too. I was banging my head against the wall trying to understand why source content was being inserted into Less map output even when that option was turned off for the Less compiler. I am baffled why this is a non-configurable option. Why assume that the content would always be inserted? And that no one would want to change that? |
@matthew-dean using paths to reference the source content can go wrong in many, many ways, and has no advantages that I'm aware of. Embedding the source content means you do not have to worry about paths whatsoever, and can always rely on having correct source maps. Larger file size is of no consequence at all in development, and if you are using source maps in production, wat. That's why I originally made the decision, which has worked nicely for any applications I have used it for. That being said, I am entirely open to arguments to the contrary as well as pull requests! |
This is actually pretty common and not an anti-pattern in my opinion. Stuff still needs to be debugged in production and debugging minified/transpiled stuff is a PITA. For example, I often use Sentry for collecting production bugs remotely. It's worth noting that sourcemaps generally aren't downloaded unless you have the dev tools open. |
I must admit I'm pretty surprised by this, but I can see how someone might think it was a good idea. Either way, I'd be happy to check out a pull request to add in this option, but I don't see it getting high enough on my priority list right now for me to work on it. |
Yup, no problem. Currently I'm using a wrapper around accord that does a few things like normalizing paths and deleting sourcesContent. Would probably make sense to push them back upstream to accord as options at some point. |
Would be great! |
any update here? |
It's on my todo list but it will probably be August before I get around to it. :-) @jescalan Where's the best place to drop you an email? |
@davej you can snag my email from my website |
Thanks! I'll drop you an email over the weekend. |
Hey @jescalan, apologies for going off-topic, did you get my email? |
@davej hi! yeah i did im so sorry i haven't responded yet. it's super awesome and i need to find time to sit down and write a good response |
No problem. Thanks for taking a look, looking forward to hearing your thoughts :-) |
Discussion began in #119 but creating a new issue instead of cluttering that PR.
I imagine when the compiler itself is outputting the
sourceContent
then the hit wouldn't be too large because the compiler is probably reading the file anyway. It's only when accord needs to do the inlining would the perf take a hit. I'll look into this over the weekend.Before I do, where would you think would be a good place to put the option? I rather like the idea of having the ability to set the option for all compilers.
I was thinking something like:
Thoughts?
The text was updated successfully, but these errors were encountered: