madskristensen / WebEssentials2013

Visual Studio extension
http://vswebessentials.com
Other
945 stars 252 forks source link

Use a baseUrl setting to reference original JS files inside a map file. #1946

Closed thitemple closed 9 years ago

thitemple commented 9 years ago

I have added a parameter baseUrl to solution settings that is used when generating JavaScript map files, so the map file will point to the original JS files using this base url.

madskristensen commented 9 years ago

@am11 what do you think?

am11 commented 9 years ago

@vintem, can you please explain what is the usecase / issue here and in your testing what was the output before and after? The physical path / URI's resolution is always tricky.

BTW, the part of code you have changed is a non-standard source-map. It is not actually a source map string, but an indicated for AjaxMin (something called debug information, the non-standard way in LibSass jargon). The real Base64 V3 .map file is generated by AjaxMin in case of bundling.

Supposedly I am missing the fact that there exists some browser which also understands the ///#source .. format to map the file, then IMO we should resolve the path w.r.t base and the current-working-directory as a secondary base path as oppose to concatenate it.

thitemple commented 9 years ago

@am11 my use case is simple actually. I have to deploy my site to a virtual directory, so my application url would be domain.com/application

So, because I'm using /Scripts/app/script.js, when deployed the map path points to this url and it cannot find the original js file.

The tests I made were: minifying the files with the configuration as is, so the map file points to /Scripts/app/.... And when I changed the configuration to use the name of my application the map file points to /ApplicationName/Scripts/app/....

That's it! If you have another suggestion to do that, I'll gladly change the way I do it, or if it is the case, change my pull request.

Thank you

am11 commented 9 years ago

@vintem, thanks for the information. It makes sense to have the way you have added it, since we can't resolve path as there is a chance that both paths (base and subject) can be absolute, based on the user-input (in .bundle file).

I have left some comments, please let me know what do you think of those. Otherwise it LGTM as a nice-to-have option. :smile:

Thanks! :+1:

madskristensen commented 9 years ago

@am11 @vintem Just let me know when this is ready to merge

thitemple commented 9 years ago

@am11 So, I tested the bundle with css and html files, and those are not generate with map files. Is this behavior correct or am I missing something?

I've never used those before.

Also, for TypeScript the map file is created automatically and it does not use the path /Scripts/file... it points directly to the original file in place. Like so:

file1.ts file1.js file1.js.map // references file1.js directly

If this is Ok and you don't have any other things to verify, there's nothing to change for those files, I'll just change the category and description as you mentioned.

am11 commented 9 years ago

Sounds good to me. Thanks for the investigation.

I'll just change the category and description as you mentioned.

Sure, please also revert the LegacySettings change.

thitemple commented 9 years ago

Done. Thanks for the help guys!

One question, any idea when a new version will be released?

madskristensen commented 9 years ago

I'll release it later this week potentially

am11 commented 9 years ago

Thanks. Please squash the commits to a single commit:

# after reverting the LegacySettings

cd into/webessentials2013
git rebase -i HEAD~4

# text editor will open
# skip the first line and replace "pick" with "f" for the next 3 lines
# save and close editor and run

git push -f
thitemple commented 9 years ago

@am11 sorry about the commented line of code, I was testing and forgot to remove it.

am11 commented 9 years ago

No problem. Thanks for squashing the commits. :+1:

@madskristensen, this looks good to me. Ready for merge. :ship: