accord icon indicating copy to clipboard operation
accord copied to clipboard

Make `sourcesContent` optional

Open davej opened this issue 8 years ago • 14 comments

Discussion began in https://github.com/jenius/accord/pull/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:

accord.setOptions({
  sourceContent: false,
  pathsRelativeTo: '/Users/dave/github/accord'
});

Thoughts?

davej avatar Dec 11 '15 15:12 davej

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 :grinning:

jescalan avatar Dec 12 '15 03:12 jescalan

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 avatar May 17 '16 19:05 matthew-dean

@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!

jescalan avatar May 17 '16 19:05 jescalan

if you are using source maps in production, wat.

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.

davej avatar May 18 '16 07:05 davej

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.

jescalan avatar May 18 '16 13:05 jescalan

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.

davej avatar May 18 '16 14:05 davej

Would be great!

jescalan avatar May 18 '16 15:05 jescalan

any update here?

jescalan avatar Jul 14 '16 20:07 jescalan

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 avatar Jul 14 '16 22:07 davej

@davej you can snag my email from my website

jescalan avatar Jul 14 '16 22:07 jescalan

Thanks! I'll drop you an email over the weekend.

davej avatar Jul 14 '16 22:07 davej

Hey @jescalan, apologies for going off-topic, did you get my email?

davej avatar Jul 28 '16 10:07 davej

@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

jescalan avatar Jul 28 '16 14:07 jescalan

No problem. Thanks for taking a look, looking forward to hearing your thoughts :-)

davej avatar Jul 28 '16 14:07 davej