Skip to content
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

Fixes regressions introduced in previous merges #455

Merged
merged 9 commits into from
Aug 8, 2014

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Jul 29, 2014

Fixes a few issues introduced in recently merged pull requests.

#441: resolve_relative_path would always return lowercase paths on windows, which will not work if one would deploy the files to a linux server (or any other case sensitive filesystem). Implemented a better and IMO correct way to handle this. Also introduced a preprocessor directive FS_CASE_SENSITIVE so that the behaviour could be overruled on compile time.

#440: sourceMappingURL is always added even if source_map_file was not set to anything at all.

aa02300 - omit_source_map_url
61edd14 - FS Case Sensitivity
@am11
Copy link
Contributor

am11 commented Jul 29, 2014

Thank you @mgreter. I was just about to install everything on my new system to fix this. 👍

@am11
Copy link
Contributor

am11 commented Jul 29, 2014

@mgreter,

There still needs to be few fixes to be made.

The idea really is:

  • the primary source file (to be compiled) can be at different location than the output-path.
  • the source-map file can be at different location than the output file.

Both of the above assertions aren't complied by libsass at the moment.

For that matter:

First, we need to store initializers.output_path() in class variable and then initialize source_map with resolve_relative_path(output_path, source_map_file, cwd) (in a ctor body -- after initiating cwd).

Secondly, in format_source_mapping_url(), we need to change it to:

"/*# sourceMappingURL=" + resolve_relative_path(source_map_file, source_path, cwd) + " */";

I guess after that the API will be quite flexible and finally browsers will be able to locate correct sources with source maps generated by libsass.

Thoughts?

mgreter added 4 commits July 29, 2014 17:54
Note that this does *not* collapse x/../y sections into y.
This is by design. If /foo on your system is a symlink to
/bar/baz, then /foo/../quux is actually /bar/quux, not
/quux as a naive ../-removal would give you.
Adds canonical file resolving where appropriate.

It might should be noted that output_path does not mean
that libsass will write the result to that location. It
is merely used to create file paths relative to the output
file (which defaults to input_path with a 'css' extension).
Accounts for parent references (`..`) in `stripped_base`.
@mgreter
Copy link
Contributor Author

mgreter commented Jul 29, 2014

@am11: Libsass already resolves sources relative to the source map file. I have added some additonal commits which should do what you have asked for (sourceMappingURL relative to output_path).
You got lucky since I probably need this feature soon :)

@akhleung: With these changes paths are internally always represented with forward slashes. The make_canonical_path function should be used to sanitize paths when they come from outside or the system. It should normalize . references and repeated slashes. The result will always have forward slashes. I also added some more details to the corresponding commit messages.

mgreter added 2 commits July 29, 2014 21:28
Add a source entry for stdin. This mimics exactly the
behavior of the Google Closure Compiler on stdin data.
@am11
Copy link
Contributor

am11 commented Jul 29, 2014

@mgreter, thanks! All these changes are extremely useful. 👍

Regarding the source-map initializer issue, please see the source_map's ctor, and this is how source-map is currently initialized in context.cpp. The problem is with the "file" member of JSON, which should be the output_path relative to source_map_file. Currently it's merely displaying the file name of output_path; implying both map and output will be in same directory (which is not the case).

mgreter added 2 commits July 30, 2014 00:12
Points to working directory (ending with a slash) if no
input file path has been given (i.e. on 'compile_string').
@am11
Copy link
Contributor

am11 commented Jul 30, 2014

Thanks @mgreter! 😃 👍

@mgreter
Copy link
Contributor Author

mgreter commented Aug 5, 2014

@am11 Can you confirm that your issues are fixed by this pull request? @akhleung Not sure if you missed this one, or are there any objections regarding this?

@akhleung
Copy link

akhleung commented Aug 5, 2014

Sorry, I just haven't had time to look at it yet. Will try to get it merged soon.

@am11
Copy link
Contributor

am11 commented Aug 6, 2014

@mgreter, yes; all our path-related concerns regarding .map JSON and source maps comments are addressed by this. I have reviewed the code but haven't tried merging the commit (I am running into some PC issues, can't test it until next week). Rest assured, it looks pretty solid than the current state.

Hopefully we get the mapping issue fixed as well. I don't have right insights about the sass-compiler's internals and how it reports sources. If you guys can take a look or point me to the locations in code so I can give it a try, it will truly make a big impact and help many affected out there..

Thanks!

@akhleung
Copy link

akhleung commented Aug 7, 2014

Looked over the code and it looks good ... I'll test/merge it in the morning when I have access to my work machine.

@mgreter mgreter changed the title Fixes regressions introduces in previous merges Fixes regressions introduced in previous merges Aug 7, 2014
akhleung pushed a commit that referenced this pull request Aug 8, 2014
Fixes regressions introduced in previous merges
@akhleung akhleung merged commit b7c7443 into sass:master Aug 8, 2014
@HamptonMakes
Copy link
Member

Merge o'clock has begun.

On Fri, Aug 8, 2014 at 9:09 PM, Aaron Leung notifications@github.com
wrote:

Merged #455 #455.

Reply to this email directly or view it on GitHub
#455 (comment).

@am11
Copy link
Contributor

am11 commented Aug 8, 2014

Indeed 😃

I wish if someday node-sass will get blessed with such a thing: "Merge o' clock"... node-sass has been insanely slow lately in adapting changes from libsass, accepting PRs, releases and all those usual things which happen to a repository..

@akhleung
Copy link

akhleung commented Aug 8, 2014

Unfortunately, it looks like node-sass's maintainer is no longer able to devote time to the project:

https://twitter.com/teabass/status/494993372252495874

@am11
Copy link
Contributor

am11 commented Aug 8, 2014

Can anyone else take the throne and keep the wheel spinning?
That is the only SASS' gateway to the JS world.
All other sass npms are dead in water 🐟

@mgreter mgreter deleted the srcmap-regressions branch October 5, 2014 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants